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

posix: implement pthread_attr_getinheritsched() and pthread_attr_setinheritsched() #68470

Conversation

moonlight83340
Copy link
Contributor

This is part of the See #51211 (RFC #51211).

pthread_attr_setinheritsched() and pthread_attr_getinheritsched() are required as part of _POSIX_THREAD_PRIORITY_SCHEDULING Option Group.

For more information, please refer to https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_setinheritsched.html

Fixes #66968
Fixes #66966

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch 8 times, most recently from 46c840a to a36faff Compare February 3, 2024 15:38
@moonlight83340 moonlight83340 marked this pull request as ready for review February 4, 2024 04:26
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Feb 4, 2024
@moonlight83340
Copy link
Contributor Author

moonlight83340 commented Feb 4, 2024

What should I set on initial value in pthread_attr_init() ?
https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_setinheritsched.html
I may haven't take a good look at it but I see no mention 😓

@ycsin
Copy link
Collaborator

ycsin commented Feb 5, 2024

What should I set on initial value in pthread_attr_init() ?

It seems to be "implementation-defined" according to the spec:

When a process is created, its single thread has a scheduling policy and associated attributes equal to the policy and attributes of the process. The default scheduling contention scope value is implementation-defined. The default values of other scheduling attributes are implementation-defined.

I did a quick search on google for "default value of inheritsched" and seems like (based on my observation in the search result) most Linux OSes have the default value of PTHREAD_INHERIT_SCHED:

  • QNX Neutrino ("The default value of the thread inherit scheduling attribute is PTHREAD_INHERIT_SCHED.")
  • IBM AIX ("The default value of the inheritsched attribute is PTHREAD_INHERIT_SCHED")
  • Oracle Solaris ("might change from PTHREAD_EXPLICIT_SCHED to PTHREAD_INHERIT_SCHED in a future Solaris release")

The Apache NuttX however, has the default value of PTHREAD_EXPLICIT_SCHED

@moonlight83340
Copy link
Contributor Author

closed by error 😓

@cfriedt
Copy link
Member

cfriedt commented Feb 5, 2024

Wow @moonlight83340 - you have been very busy!

Thanks - I'm taking a look now.

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.

@moonlight83340 - nice work 👍

I think there may be some additional tests to write, and likely some additional work on the implementation of pthread_create()

Tests:

  1. Verify that the scheduling attributes of the parent thread (policy, priority, ..) are inherited by a child thread when INHERIT_SCHED is used. This may be tricky, because currently Zephyr threads are not pthreads. It might be necessary to extrapolate equivalent pthread attributes (policy, priority) from the running k_thread and compare those.

  2. Verify that the scheduling attributes specified by the pthread_attr_t are passed to the child thread.

For both of those, you should be able to create a static helper function in the same file. Something like this?

static void test_pthread_attr_set_inheritsched_common(int inheritsched);

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch 2 times, most recently from ad95b34 to ab5db34 Compare February 6, 2024 06:58
@moonlight83340
Copy link
Contributor Author

moonlight83340 commented Feb 6, 2024

@moonlight83340 - nice work 👍

I think there may be some additional tests to write, and likely some additional work on the implementation of pthread_create()

Tests:

1. Verify that the scheduling attributes of the parent thread (policy, priority, ..) are inherited by a child thread when INHERIT_SCHED is used. This may be tricky, because currently Zephyr threads are not pthreads. It might be necessary to extrapolate equivalent pthread attributes (policy, priority) from the running `k_thread` and compare those.

2. Verify that the scheduling attributes specified by the pthread_attr_t are passed to the child thread.

For both of those, you should be able to create a static helper function in the same file. Something like this?

static void test_pthread_attr_set_inheritsched_common(int inheritsched);

Still working on the test part not sure on how I will do that but I think I got it for the pthread_create() part.

I have done something like that :

	struct posix_thread *self = NULL;

	K_SPINLOCK(&pthread_pool_lock) {
		self = to_posix_thread(pthread_self());
		if (self == NULL) {
			K_SPINLOCK_BREAK;
		}
		if (self_attr.inheritsched == PTHREAD_INHERIT_SCHED) {
			t->attr.priority = self->attr.priority;
			t->attr.schedpolicy = self->attr.schedpolicy;
			t->attr.inheritsched = self->attr.inheritsched;
		}
	}

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch 3 times, most recently from e763394 to 44679a2 Compare February 11, 2024 10:04
@moonlight83340
Copy link
Contributor Author

Not sure on the error, I will check that when I have time.

@cfriedt
Copy link
Member

cfriedt commented Mar 20, 2024

@moonlight83340 - can you please update this PR?

@moonlight83340
Copy link
Contributor Author

@moonlight83340 - can you please update this PR?

I will take a look on the week ! ^^'

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch from 44679a2 to 453b350 Compare March 21, 2024 09:44
@moonlight83340
Copy link
Contributor Author

Assertion failed at CMAKE_SOURCE_DIR/src/pthread_attr.c:541: test_pthread_attr_set_inheritsched_parent_fn: pthread_create(&child, NULL, test_pthread_attr_set_inheritsched_child_fn, NULL) is non-zero

I think, I need to check on before the thread creation to found why the thread can't create another thread.

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.

It's close - a suggestion to maybe simplify things

lib/posix/options/pthread.c Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member

cfriedt commented Mar 29, 2024

@moonlight83340 - can you apply this patch on top of your current commit? (please also fixup the code and test commits seperately)
https://pastebin.com/a7x4GXmg

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch 4 times, most recently from bfa882a to 4716053 Compare March 29, 2024 10:11
@cfriedt
Copy link
Member

cfriedt commented Mar 29, 2024

@moonlight83340 - fixup commits are somewhat special. Check this out. Also, the Signed-off-by line is automatically added to commit messages with git commit --signoff or git commit -s.

See also Contribution Workflow.

@moonlight83340
Copy link
Contributor Author

@moonlight83340 - fixup commits are somewhat special. Check this out. Also, the Signed-off-by line is automatically added to commit messages with git commit --signoff or git commit -s.

See also Contribution Workflow.

I had never used fixup in git command, so thank you !

@moonlight83340
Copy link
Contributor Author

@moonlight83340 - fixup commits are somewhat special. Check this out. Also, the Signed-off-by line is automatically added to commit messages with git commit --signoff or git commit -s.

See also Contribution Workflow.

Should I rebase ? git rebase --autosquash --interactive base

@cfriedt
Copy link
Member

cfriedt commented Mar 29, 2024

@moonlight83340 - you should also rebase on main if you haven't already. This PR is a bit behind.

git checkout main
git fetch -p && git pull
git checkout getinheritsched_setinheritsched
git rebase main

For reference, my rebased version of this PR shows over 500 commits and nearly 2000 files changed
moonlight83340/zephyr@getinheritsched_setinheritsched...cfriedt:zephyr:blah

@cfriedt
Copy link
Member

cfriedt commented Mar 29, 2024

@moonlight83340 - this would be a more accurate test, I think.

https://pastebin.com/1mm733Lw

@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch from 4716053 to 0d1e492 Compare March 30, 2024 03:04
Implement `pthread_attr_setinheritsched()` and
`pthread_attr_getinheritsched()`are required
as part of _POSIX_THREAD_PRIORITY_SCHEDULING Option Group.

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
`pthread_attr_getinheritsched()` and
`pthread_attr_setinheritsched()` are now implemented,
mark it so.

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch from 0d1e492 to 2886142 Compare March 30, 2024 03:05
…attr

Add tests for `pthread_attr_setinheritsched()`
and `pthread_attr_getinheritsched()`

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
@moonlight83340 moonlight83340 force-pushed the getinheritsched_setinheritsched branch from 2886142 to 77bc598 Compare March 30, 2024 03:16
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.

LGTM - thanks 👍

@moonlight83340
Copy link
Contributor Author

LGTM - thanks 👍

Thanks to you, I will carefully check your patch.
Hope to get the chance to continue my contribution !

@fabiobaltieri fabiobaltieri merged commit c877cd9 into zephyrproject-rtos:main Apr 1, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

posix: implement pthread_attr_setinheritsched() posix: implement pthread_attr_getinheritsched()
5 participants