Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mm: Remove mm_heap_impl_s struct #4068

Merged
merged 1 commit into from
Jul 7, 2021
Merged

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

since it's more simple to make mm_heap_s opaque outside of mm

Impact

mm_heap_s become opaque struct

Testing

ostest

@davids5
Copy link
Contributor

davids5 commented Jul 6, 2021

@xiaoxiang781216 I just noticed that the change adding the name is not reflected in the mm/README.txt nor the titale blocks

/****************************************************************************
 * Name: mm_initialize
 *
 * Description:
 *   Initialize the selected heap data structures, providing the initial
 *   heap region.
 *
 * Input Parameters:
 *   heap      - The selected heap
 *   heapstart - Start of the initial heap region
 *   heapsize  - Size of the initial heap region
 *
 * Returned Value:
 *   None
 *
 * Assumptions:
 *
 ****************************************************************************/

Copy link
Contributor

@davids5 davids5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiang781216 -

This looks good.

Comment on lines 57 to 60
/* Initialize the user heap if not yet */

umm_try_initialize();

Copy link
Member

@Ouss4 Ouss4 Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Initialize the user heap if not yet */
umm_try_initialize();
/* Initialize the user heap if it wasn't yet. */
umm_try_initialize();

Why is this required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel mode doesn't call umm_initialize in nx_start(it's impossible from technique since each process has a dedicated heap instance). Before this patch, sbrk will initialize the heap when malloc fail first time and fallback to sbrk, but we have to initialize explicitly here because tg_heap change to pointer and then no place to save the flag before we let g_heap point to some memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is fixed, @Ouss4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

@xiaoxiang781216
Copy link
Contributor Author

@xiaoxiang781216 I just noticed that the change adding the name is not reflected in the mm/README.txt nor the titale blocks

/****************************************************************************
 * Name: mm_initialize
 *
 * Description:
 *   Initialize the selected heap data structures, providing the initial
 *   heap region.
 *
 * Input Parameters:
 *   heap      - The selected heap
 *   heapstart - Start of the initial heap region
 *   heapsize  - Size of the initial heap region
 *
 * Returned Value:
 *   None
 *
 * Assumptions:
 *
 ****************************************************************************/

Done.

Copy link
Member

@Ouss4 Ouss4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

it's more simple to make mm_heap_s opaque outside of mm

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: I5c8e435f6baba6d22b10c5f7e8d9191104fb5af2
@davids5 davids5 merged commit 76cdd5c into apache:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants