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

signal: add support for realtime signals on illumos #7029

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Dec 11, 2024

Motivation

Completing the work started in #6995 -- the SIGRTMIN and SIGRTMAX APIs were added for illumos in libc 0.2.168. They were also added for Solaris, but out of caution I don't want to turn this on until someone verifies that realtime signals work as expected there.

Also added a test for realtime signals which can be used while porting to other platforms in the future. The test passes successfully on both Linux and illumos (tested locally).

@sunshowers
Copy link
Contributor Author

Hmm, I guess qemu (on some targets?) doesn't support realtime signals? Thoughts on whether/how to keep the test?

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-signal Module: tokio/signal labels Dec 11, 2024
@Darksonn
Copy link
Contributor

cc @hawkw for illumos
cc @ipetkov for signal

@Noah-Kennedy
Copy link
Contributor

@sunshowers there's CI jobs failing right now.

I can review this once CI passes.

I'm operating under the assumption here that you have tested this on Illumos (you probably have a better test environment for that OS than I do 🤪).

@sunshowers
Copy link
Contributor Author

sunshowers commented Dec 14, 2024

@sunshowers there's CI jobs failing right now.

I can review this once CI passes.

See my question in #7029 (comment). It looks like this may be a deficiency in qemu cross-testing exposed by my test, and I would love advice on whether and how to retain the test.

(Yeah, I've tested on illumos, as mentioned in the PR summary.)

@Noah-Kennedy
Copy link
Contributor

Sorry, I missed that.

You can just disable it on the broken targets for now.

@sunshowers
Copy link
Contributor Author

Thanks -- is there a general way to determine in the test suite whether a test is being run under qemu? The issue isn't with the targets, but with the fact that qemu is involved.

@Noah-Kennedy
Copy link
Contributor

Nothing in the harness afaik, and otomh I am not sure what the best way to determine if in qemu is.

@sunshowers
Copy link
Contributor Author

sunshowers commented Dec 14, 2024

All right, I think I see what's going on.

Looking at it more deeply on my local machine -- while cross-testing from x86_64-unknown-linux-gnu to aarch64-unknown-linux-gnu, SIGRTMIN returns 34 and SIGRTMAX returns 64 (that is a total of 31 signals). But it looks like specifically, sending signal 62, 63 and 64 all fail with EINVAL. The range 34..=61 works fine.

It looks like this is documented in https://wiki.qemu.org/ChangeLog/9.2#signals as being expected. 28 realtime signals are available, which matches the range 34..=61. The documentation says the last 4 signals are invalid, but that's assuming there are 32 signals -- in reality, qemu only returns 31 signals, so the last 3 are invalid.

Based on this, I think I'm just going to cap it at SIGRTMIN + 27 (inclusive, i.e. 28 signals).

The API was added in libc 0.2.168.

Also added a test for realtime signals.
@sunshowers
Copy link
Contributor Author

All right, let's see how this goes.

@sunshowers
Copy link
Contributor Author

Looks like the tests passed! The one failure looks unrelated/flaky maybe.

@Noah-Kennedy
Copy link
Contributor

i'm kicking the failing jobs

@Noah-Kennedy Noah-Kennedy merged commit aa7e0ce into tokio-rs:master Dec 14, 2024
82 checks passed
@sunshowers sunshowers deleted the realtime-illumos branch December 14, 2024 04:33
@Noah-Kennedy
Copy link
Contributor

@sunshowers merged 🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-signal Module: tokio/signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants