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

start-stop-daemon: fix setting scheduler with musl #690

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

ncopa
Copy link
Contributor

@ncopa ncopa commented Feb 5, 2024

musl sched_setscheduler() is only a stub in musl and the glibc implementation does "the wrong thing". Use pthread_setschedparam instead.

ref: https://www.openwall.com/lists/musl/2016/03/01/5
fixes: #689

@ncopa
Copy link
Contributor Author

ncopa commented Feb 5, 2024

Seems like we either need to add -lpthread for FreeBSD or have an #ifdef __linux__. Not sure what is preferred.

@@ -1123,7 +1124,11 @@ int main(int argc, char **argv)
if (sched_prio == -1)
sched.sched_priority = sched_get_priority_min(scheduler_index);

#ifdef __linux__
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To quote @richfelker

The reason it doesn't do anything is that Linux does not provide a way
to set scheduling parameters for a process, only for threads. The
sched_setscheduler syscall is documented as taking a pid but actually
takes a thread id and only operates on that thread. glibc just ignores
this and provides sched_* functions that do the wrong thing.

I read that as it is a problem with sched_setscheduler on Linux and other OSes may not need this workaround.

Copy link
Member

Choose a reason for hiding this comment

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

for a single threaded process, like s-s-d, pid==tid. so it sounds like sched_setscheduler already works correctly.

Choose a reason for hiding this comment

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

for a single threaded process, like s-s-d, pid==tid. so it sounds like sched_setscheduler already works correctly.

It's a no-op/always-fail, which is working correctly, per the specified requirements, but it's probably not what you want. Calling the sched_setscheduler function would set the process scheduling, but there's no such thing on Linux (it's optional POSIX functionality). However the syscall by that name, and the glibc sched_setscheduler function, target a thread, behaving like pthread_setschedparam but with a kernel TID as the target instead of a pthread_t. They do not target a process. The glibc function probably does what you want, but that's because it's doing the wrong thing.

If that is what you want, the right way to do it is by calling pthread_setschedparam on pthread_self().

Copy link
Member

Choose a reason for hiding this comment

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

It's a no-op/always-fail

you've made that choice in your implementation

Calling the sched_setscheduler function would set the process scheduling, but there's no such thing on Linux

this seems to be bleeding into internal details. from a single-threaded process pov, the API works with glibc (and other C libraries afaik). the program wants to set scheduling details for itself, and sched_setscheduler does that.

i get that in a multithreaded process, there aren't separate process & thread level scheduling in Linux that threads would be able to pick between via contention scope. but we aren't talking about multithreaded processes here.

it's optional POSIX functionality

pthread_setschedparam is also optional functionality, both at the pthreads level, and at the pthreads scheduling level. so this argument doesn't really hold water.

it also requires compiler & linker probing to use properly -- which we don't do because nothing in openrc relies on pthreads today.

The glibc function probably does what you want, but that's because it's doing the wrong thing.

it's a funny argument when you then advocate for changing openrc to do exactly what glibc is doing, but with a diff function name.

Copy link
Member

Choose a reason for hiding this comment

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

again, what you describe is internal details bleeding out. in a single threaded process, which is the only thing we're concerned with here, linux+glibc's sched_setscheduler behaves correctly. the process expects certain runtime behavior, and it gets that. how the runtime decides to store or calculate those settings internally is irrelevant.

you are correct that in a multithreaded process, sched_setscheduler only changes the specified thread, not the overall process. but we aren't using threads, so the deficiency is irrelevant, as is pointing out the breakdown when additional threads are created. in a single threaded process, the name & behavior are aligned.

this PR proposes making changes for all Linux systems in a way that doesn't work, and is incomplete even for musl -- i'm guessing sched_setparam is similarly broken. if we wanted to make a change in here to accommodate musl, we would need to probe the C library to figure out it's actually musl, and then put this rewrite logic into a quirks file. inline ifdefs make the code less maintainable (yes, i know s-s-d has that already).

Choose a reason for hiding this comment

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

again, what you describe is internal details bleeding out. in a single threaded process, which is the only thing we're concerned with here, linux+glibc's sched_setscheduler behaves correctly.

Once again, no it does not. It does what you want, which is very different from behaving correctly. Behaving correctly would be setting the process priority, which is specified to be ignored unless one or more threads in the process have had their scheduling scope set to PTHREAD_SCOPE_PROCESS (or if this is the implementation-defined default). Since Linux does not support PTHREAD_SCOPE_PROCESS, this is never the case.

this PR proposes making changes for all Linux systems in a way that doesn't work, and is incomplete even for musl -- i'm guessing sched_setparam is similarly broken. if we wanted to make a change in here to accommodate musl, we would need to probe the C library to figure out it's actually musl, and then put this rewrite logic into a quirks file. inline ifdefs make the code less maintainable (yes, i know s-s-d has that already).

No, all you need to do is replace the sched_setscheduler call targering the caller's pid with pthread_setschedparam targeting pthread_self(). This is the portable (works on glibc and everywhere else) way to do what you're currently achieving by assuming glibc's implementation of sched_setscheduler, which does the wrong (but wanted) thing. Note that the glibc folks even acknowledged that it's doing the wrong thing and temporarily fixed it, but reverted the fix because it was a breaking change for applications which expect the wrong glibc behavior...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

glibc's implementation of sched_setscheduler, which does the wrong (but wanted) thing. Note that the glibc folks even acknowledged that it's doing the wrong thing and temporarily fixed it, but reverted the fix because it was a breaking change for applications which expect the wrong glibc behavior...

Does this mean that FreeBSD does something we don't want in this case? (assuming it follows the POSIX spec correctly?)

It sounds like it would be better to use pthread_setchedparam also for FreeBSD (and add -lpthread)

Choose a reason for hiding this comment

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

Does this mean that FreeBSD does something we don't want in this case? (assuming it follows the POSIX spec correctly?)

Maybe. It's possible they support the process scheduling scope and use it by default, in which case sched_setscheduler would have a different but possibly more desirable behavior than the glibc behavior. Or it's possible that it does something different but not-quite-right. I don't know enough about FreeBSD (or other BSDs for that matter) kernel or threads implementation to answer.

It sounds like it would be better to use pthread_setchedparam also for FreeBSD

It would at least ensure you're getting more uniform behavior.

(and add -lpthread)

I suspect this is the part they don't want to do, for whatever reason. I guess it could be avoided on Linux by special-casing Linux and using the raw syscall there, but yes just using -lpthread and pthread_setscheduler would be the clean cross-platform solution.

Copy link
Member

Choose a reason for hiding this comment

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

reading over this, seems like indeed glibc does something wrong that just happens to work

i'm in favor of using the pthread solution as, even tho we're single threaded, it's the more sound solution (does what we want, with the expected standardized behavior)

will take a look into the code for this soon

@ncopa
Copy link
Contributor Author

ncopa commented Feb 19, 2024

I updated it to use pthread_setschedparam everywhere so we avoid depending on implementation specific behaviour.

Linux does not provide a way to set scheduling parameters for a process,
only for threads, so musl sched_setscheduler() is only a stub and the
glibc implementation does "the wrong thing".

Use pthread_setschedparam everywhere so we don't depend on any
implementation specific behavior.

ref: https://www.openwall.com/lists/musl/2016/03/01/5
fixes: OpenRC#689
@navi-desu navi-desu merged commit c325954 into OpenRC:master Jul 24, 2024
5 checks passed
@navi-desu
Copy link
Member

thanks!

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.

openrc start-stop-daemon scheduler param is not working with musl libc
4 participants