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

finalize-staged: Don't listen to SIGTERM, just let kernel exit us #2704

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

cgwalters
Copy link
Member

Followup from discussion in
#2544 (comment)

This is more efficient; no need to have the kernel context switch
us in at shutdown time just so we can turn around and call
exit().

@cgwalters cgwalters marked this pull request as ready for review August 30, 2022 20:24
@openshift-ci
Copy link

openshift-ci bot commented Aug 30, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

g_unix_signal_add (SIGTERM, sigterm_cb, &running);
g_print ("Waiting for SIGTERM\n");
while (running)
while (TRUE)
g_main_context_iteration (NULL, TRUE);
Copy link
Member

Choose a reason for hiding this comment

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

If the loop isn't doing anything, why not just use pause()? That's actually exactly what sleep infinity does. Seems silly to have glib setup an event loop that does nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we're just optimizing a no-op at this point, but yes, done 😄
I didn't know about the pause system call actually. It looks like it may be x86 only; glibc has a fallback that just uses ppoll() https://github.com/bminor/glibc/blob/02ca25fef2785974011e9c5beecc99b900b69fd7/sysdeps/unix/sysv/linux/pause.c#L31 with NULLs.

Copy link
Member

Choose a reason for hiding this comment

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

I ran strace on sleep infinity and that's where it blocked. I think it is provided most everywhere on linux since it's in POSIX. pause is defined in most syscall tables in the kernel and there's a generic implementation.

Followup from discussion in
ostreedev#2544 (comment)

This is more efficient; no need to have the kernel context switch
us in at shutdown time just so we can turn around and call
`exit()`.
@cgwalters cgwalters force-pushed the finalize-no-sigterm branch from 0081bf3 to 683e4ef Compare August 30, 2022 21:50
@dbnicholson dbnicholson enabled auto-merge August 30, 2022 21:54
@dbnicholson dbnicholson merged commit eed9e9f into ostreedev:main Aug 30, 2022
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.

2 participants