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

pthread: facilitate dynamically allocated thread stacks #29029

Closed
wants to merge 2 commits into from
Closed

pthread: facilitate dynamically allocated thread stacks #29029

wants to merge 2 commits into from

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Oct 8, 2020

This change allows users to call pthread_create(3) with the pthread_attr_t argument equal to NULL, or with the pthread_attr_t argument specifying a NULL stack.

If either of the above to requirements are met, then a stack will be heap-allocated internally and freed again after the thread has terminated.

This helps the Zephyr implementation of pthread_create(3) become more compliant with the normative spec.

Fixes #25973

@cfriedt cfriedt marked this pull request as draft October 8, 2020 00:54
@github-actions github-actions bot added area: API Changes to public APIs area: Kernel area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test labels Oct 8, 2020
@cfriedt cfriedt changed the title WIP: posix: heap-allocated thread stacks posix: heap-allocated thread stacks Oct 23, 2020
@cfriedt cfriedt changed the title posix: heap-allocated thread stacks pthread: facilitate dynamically allocated thread stacks Oct 23, 2020
@cfriedt cfriedt marked this pull request as ready for review October 23, 2020 03:16
lib/posix/pthread.c Outdated Show resolved Hide resolved
lib/posix/pthread.c Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member Author

cfriedt commented Oct 23, 2020

Looks like this assertion fails in some tests:

	atomic_val_t old_val = atomic_set(&new_thread->base.cookie,
					  THREAD_COOKIE);
	/* Must be garbage or 0, never already set. Cleared at the end of
	 * z_thread_single_abort()
	 */
	__ASSERT(old_val != THREAD_COOKIE,
		 "re-use of active thread object %p detected", new_thread);

and there are some (predictable) MPU / MMU faults due to permissions.

Will address those issues shortly.

Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

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

Does it also make sense to or with K_INHERIT_PERMS in this case? cc: @andrewboie

K_INHERIT_PERMS is a userspace thing, so I do not believe so. the posix layer doesn't support user mode. I might just not be seeing what you mean here though, can you elaborate?

Looks like this assertion fails in some tests:

How are you managing when it's safe to free stacks back to the heap? are you using k_thread_join() or is it the fn_abort hook? it should be safe to free them at that time. that assertion fails when you call k_thread_create() on a thread object that is already active and not exited yet.

lib/posix/Kconfig Show resolved Hide resolved
lib/posix/pthread.c Outdated Show resolved Hide resolved
lib/posix/pthread.c Outdated Show resolved Hide resolved
@maxbachmann
Copy link
Contributor

K_INHERIT_PERMS is a userspace thing, so I do not believe so. the posix layer doesn't support user mode. I might just not be seeing what you mean here though, can you elaborate?

Is this documented somewhere? Since I could not find this mentioned here: https://docs.zephyrproject.org/latest/guides/portability/posix.html and no #ifndef Userspace around the code either I expected it to work in Userspace.

k_heap has a facility for aligned allocations you should be using that instead of k_malloc to reserve memory

Which facility for aligned allocations does it has? I could only find k_heap_alloc which does not allow to specify the alignment, while sys_heap has a sys_heap_aligned_alloc function.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 24, 2020

Does it also make sense to or with K_INHERIT_PERMS in this case? cc: @andrewboie

K_INHERIT_PERMS is a userspace thing, so I do not believe so. the posix layer doesn't support user mode. I might just not be seeing what you mean here though, can you elaborate?

I think I was somewhat confused and half awake when I made that comment and was conflating userspace with MMU / MPU protection.

Looks like this assertion fails in some tests:

How are you managing when it's safe to free stacks back to the heap? are you using k_thread_join() or is it the fn_abort hook? it should be safe to free them at that time. that assertion fails when you call k_thread_create() on a thread object that is already active and not exited yet.

Am using the fn_abort() hook.

Seems strange that this assertion would be triggered - will keep looking into it.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 24, 2020

I'll likely focus on eliminating MPU-related bugs in mps2_an385 for now, due to a bug in QEMU that prevents me (on my current machine) from debugging qemu_x86_64.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 24, 2020

I'll likely focus on eliminating MPU-related bugs in mps2_an385 for now, due to a bug in QEMU that prevents me (on my current machine) from debugging qemu_x86_64.

Ah, I think I see what's happening. Compensation for alignment and guard areas is happening in two places: pthread_create() and setup_thread_stack(). I will need to adjust pthread_create() code to pass in the uncompensated pointer, which should, of course, be aligned.

I think k_heap is not exactly what we're looking for in this case, since we want to allocate from the same, general memory area that k_malloc uses, and to allow the rest of the system to recycle that memory as well.

A thread can dynamically allocate a chunk of heap memory by calling k_malloc(). The address of the allocated chunk is guaranteed to be aligned on a multiple of pointer sizes. If a suitable chunk of heap memory cannot be found NULL is returned.

The issue here is that k_malloc() will only guarantee to align to 4 byte addresses on 32-bit systems and 8 byte addresses on 64-bit systems. However, thread stacks are often required to be aligned to 8 byte addresses, even on something like cortex-m3. Maybe there are other arches that have more strict alignment criteria.

Would it be realistic to ask for something like k_malloc_aligned(size_t alignment, size_t size) that mirrors the standard C11 call aligned_alloc() or posix_memalign(3)? I guess I could try to add it.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 24, 2020

Would it be realistic to ask for something like k_malloc_aligned(size_t alignment, size_t size) that mirrors the standard C11 call aligned_alloc() or posix_memalign(3)? I guess I could try to add it.

Created issue #29519

@cfriedt
Copy link
Member Author

cfriedt commented Oct 25, 2020

k_heap has a facility for aligned allocations you should be using that instead of k_malloc to reserve memory

Which facility for aligned allocations does it has? I could only find k_heap_alloc which does not allow to specify the alignment, while sys_heap has a sys_heap_aligned_alloc function.

@maxbachmann - I think you've really hit on an important topic. From the perspective of an application, it's using the same heap that "malloc" is using, which was a sys_mem_pool. From what I have gathered, performing aligned allocations was not even considered in the design of sys_mem_pool. It is a buddy allocator, with some additional features, which means that it's fast. Unfortunately, it is non-obvious (to me) how to modify the sys_mem_pool implementation to support aligned allocations.

If you're interested in this topic though, I would suggest checking out #29529. I've added you as a reviewer.

@cfriedt
Copy link
Member Author

cfriedt commented Oct 28, 2020

This one is blocked on #29529, which (I believe) is blocked on #28611.

@andrewboie
Copy link
Contributor

andrewboie commented Oct 29, 2020

Unfortunately, it is non-obvious (to me) how to modify the sys_mem_pool implementation to support aligned allocations.

Why would you do this?

I already told you sys_mem_pool and k_mem_pool are being removed from the kernel. #28611
Stop basing anything on *_mem_pool, it's going away. If the intention is to draw from the system heap, adding k_malloc_aligned is your best bet.

@cfriedt

This comment has been minimized.

@cfriedt cfriedt self-assigned this Nov 20, 2020
@cfriedt cfriedt marked this pull request as draft November 20, 2020 18:19
@cfriedt
Copy link
Member Author

cfriedt commented Nov 20, 2020

Leaving this as a draft until #30003, #28611, and #29529 are merged.

This change allows users to call pthread_create(3) with
the pthread_attr_t argument equal to NULL, or with the
pthread_attr_t argument specifying a NULL stack but a
custom stack size.

If either of the above to requirements are met, then
a stack will be heap-allocated internally and
freed again after the thread has terminated.

This makes the Zephyr implementation of pthread_create(3)
more compliant with the normative spec.

Fixes #25973

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
Tests for dynamically allocated POSIX thread stacks.

Fixes #25973

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt cfriedt marked this pull request as ready for review December 28, 2020 00:34
@cfriedt cfriedt marked this pull request as draft December 28, 2020 01:08
@cfriedt cfriedt closed this Jan 25, 2021
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: Kernel area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lib: posix: dynamic stack support for pthread_create()
3 participants