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

kernel: userspace: aligned memory allocation for dynamic objects #30762

Merged

Conversation

dcpleung
Copy link
Member

This allows allocating dynamic kernel objects with memory alignment
requirements. The first candidate is for thread objects for x86
and x86_64 where saving/restoring SSE/FPU registers require strict
alignment.

Fixes #29589
Fixes #29629

@dcpleung dcpleung requested a review from nashif December 16, 2020 01:26
@github-actions github-actions bot added area: API Changes to public APIs area: Kernel labels Dec 16, 2020
@dcpleung dcpleung linked an issue Dec 16, 2020 that may be closed by this pull request
@dcpleung dcpleung force-pushed the dynamic_kthread_alloc_alignment branch from c93df28 to 33d439e Compare December 17, 2020 00:29
kernel/userspace.c Outdated Show resolved Hide resolved
@dcpleung dcpleung force-pushed the dynamic_kthread_alloc_alignment branch from 33d439e to 1825b70 Compare December 17, 2020 03:29
@github-actions github-actions bot added the area: X86 x86 Architecture (32-bit) label Dec 17, 2020
@cfriedt
Copy link
Member

cfriedt commented Dec 17, 2020

Looks like there is some potential conflict with #30363 which has been open for a while in preparation for dynamic pthtread stacks (#29029, which has been there for quite a long time too).

@dcpleung - I wonder if this ticket could focus more on dynamic kobject creation while #30363 could focus more on allocation.

Please let me know if you can think of a better alternative.

@dcpleung
Copy link
Member Author

Looks like there is some potential conflict with #30363 which has been open for a while in preparation for dynamic pthtread stacks (#29029, which has been there for quite a long time too).

@dcpleung - I wonder if this ticket could focus more on dynamic kobject creation while #30363 could focus more on allocation.

Please let me know if you can think of a better alternative.

Once your PR is merged, I can rebase this on top of your changes.

kernel/mempool.c Outdated Show resolved Hide resolved
kernel/mempool.c Outdated Show resolved Hide resolved
include/sys/kobject.h Outdated Show resolved Hide resolved
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Looks good - just a slight change request to have consistent naming. Are any additional tests required?

@npitre
Copy link
Collaborator

npitre commented Jan 12, 2021 via email

@dcpleung
Copy link
Member Author

On Tue, 12 Jan 2021, Christopher Friedt wrote: + return z_dynamic_object_aligned_create(1, size); I think elsewhere that sizeof(void*) is used in place of an unspecified alignment because it's a reasonable safe default alignment.
Personally, I'd prefer using 0 when no particular alignment is necessary. The code will already take care to return memory with the minimum alignment for all basic types for a given platform in that case.

Hm... sys_heap_aligned_alloc() with alignment 0 simply calls sys_heap_alloc() which has no minimal alignment (AFAIK). So probably better to pass a value over.

@dcpleung dcpleung force-pushed the dynamic_kthread_alloc_alignment branch from b5744e0 to 0204c01 Compare January 12, 2021 15:20
@dcpleung
Copy link
Member Author

Looks good - just a slight change request to have consistent naming. Are any additional tests required?

Updated the naming and alignment value.

@npitre
Copy link
Collaborator

npitre commented Jan 12, 2021 via email

@dcpleung dcpleung force-pushed the dynamic_kthread_alloc_alignment branch from 0204c01 to d096311 Compare January 12, 2021 16:00
@dcpleung
Copy link
Member Author

On Tue, 12 Jan 2021, Daniel Leung wrote: Hm... sys_heap_aligned_alloc() with alignment 0 simply calls sys_heap_alloc() which has no minimal alignment (AFAIK). So probably better to pass a value over.
It sure does. sys_heap_alloc() must have minimal alignment for the platform no matter what. And I know it does.

Changed the value to 0. There is another issue where they reported the minimal alignment was 4, but I recalled it as 1, hence the confusion.

@npitre
Copy link
Collaborator

npitre commented Jan 12, 2021 via email

This adds a new z_thread_aligned_alloc() to do memory allocation
with required alignment.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
This allows allocating dynamic kernel objects with memory alignment
requirements. The first candidate is for thread objects where,
on some architectures, it must be aligned for saving/restoring
registers.

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
x86 and x86_64 require certain alignment in the k_thread struct
since the buffer to save/restore FPU/SSE registers requires
strict alignment.

Fixes zephyrproject-rtos#29589
Fixes zephyrproject-rtos#29629

Signed-off-by: Daniel Leung <daniel.leung@intel.com>
@dcpleung dcpleung force-pushed the dynamic_kthread_alloc_alignment branch from d096311 to 950bf7e Compare January 12, 2021 16:26
@dcpleung
Copy link
Member Author

On Tue, 12 Jan 2021, Daniel Leung wrote: Changed the value to 0. There is another issue where they reported the minimal alignment was 4, but I recalled it as 1, hence the confusion.
Thanks. There is a 1 in z_thread_malloc() that could be turned into a 0 as well for consistency.

Changed that too.

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

Works for me

@andrewboie andrewboie merged commit 7a5f9e8 into zephyrproject-rtos:master Jan 13, 2021
@dcpleung dcpleung deleted the dynamic_kthread_alloc_alignment branch January 13, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Base OS Base OS Library (lib/os) area: Kernel area: Userspace Userspace area: X86 x86 Architecture (32-bit)
Projects
None yet
8 participants