posix + kernel: support for realtime signals #74918

cfriedt · 2024-06-25T02:47:35Z

This was required for practicality

ring_buffer: support static rings

These add support to the kernel

kernel: initial support for signals
tests: kernel: add tests for k_sigqueue() and k_sigtimedwait()
doc: kernel: services: add a doc about signals

These complete the POSIX_REALTIME_SIGNALS Option Group (and _POSIX_REALTIME_SIGNALS Option)

posix: add realtime signal support
tests: posix: common: add tests for realtime signals
doc: posix: mark realtime signals as supported 🎉

These add pthread_kill() to (finally) complete the POSIX_THREADS_BASE Option Group and _POSIX_THREADS Option

posix: threads: implement pthread_kill()
tests: posix: headers: test for pthread_kill()
doc: posix: mark posix threads as supported 🎉

Kernel Doc Preview
POSIX Doc Preview

Fixes #59943
Fixes #74993
Fixes #74994
Fixes #74995

cfriedt · 2024-06-25T15:03:37Z

Works on my desk ¯\(ツ)/¯

twister --retry-failed 3 -T tests/posix/common -T tests/kernel/signal/
INFO    - Using Ninja..
INFO    - Zephyr version: v3.7.0-rc1-12-gc284b50da617
INFO    - Using 'zephyr' toolchain.
INFO    - Selecting default platforms per test case
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/testplan.json
INFO    - JOBS: 32
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:  562/ 562  100%  skipped:  272, failed:    0, error:    0
INFO    - 16 test scenarios (562 test instances) selected, 272 configurations skipped (246 by static filter, 26 at runtime).
INFO    - 290 of 562 test configurations passed (100.00%), 0 failed, 0 errored, 272 skipped with 0 warnings in 696.02 seconds
INFO    - In total 23167 test cases were executed, 27603 skipped on 41 out of total 757 platforms (5.42%)
INFO    - 288 test configurations executed on platforms, 2 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

cfriedt · 2024-08-10T03:06:05Z

removed reference to non-existent timeout parameter in k_sig_queue() doxygen

cfriedt · 2024-08-10T03:12:55Z

Fixed up references in signals.rst

cfriedt · 2024-08-10T12:40:19Z

I'm still seeing weirdness in the Doxygen output. Not entirely sure how to fix this. @kartben - have you ever seen this before?

kartben · 2024-08-10T12:51:23Z

I'm still seeing weirdness in the Doxygen output. Not entirely sure how to fix this. @kartben - have you ever seen this before?

Is there a reason you're using @headerfile? Looks like the likely culprit (not sure why tho)

cfriedt · 2024-08-10T13:07:04Z

Is there a reason you're using @headerfile? Looks like the likely culprit (not sure why tho)

Actually, I added @headerfile thinking that would solve the problem, but it didn't help. With or without it was the same.

Then I reversed where @addtogroup and @defgroup were for the k_sig_apis group, and it half-worked. It now shows #include <signal.h> which is slightly correct. I think it would be preferable to use #include <zephyr/kernel.h> since kernel headers shouldn't be included directly.

Ahh... I see it's the same situation already for spinlocks.

At least it's not only a problem here, so I think my next push should be OK.

Have a nice weekend. Please take a look when the signals are green again, but no rush.

kartben · 2024-08-10T13:07:43Z

@cfriedt if you want to use @headerfile tags (which I don't think you need, but I might be missing something), then double check the correct syntax for sharp brackets: https://www.doxygen.nl/manual/commands.html#cmdheaderfile

Move initializers to a common macro and provide static variants. Signed-off-by: Chris Friedt <[email protected]>

This changes adds initial support for signals. Note: this feature is experimental. The primary use-case is intended to be real-time signals. Currently, there is no asynchronous signal delivery. Signed-off-by: Chris Friedt <[email protected]>

Add tests for realtime signals. Signed-off-by: Chris Friedt <[email protected]>

Add a document describing the signal API. Provide details on * manipulating signal sets * manipulating the signal mask of the current thread * sending (queuing) a signal * receiving (waiting-on) a signal Signed-off-by: Chris Friedt <[email protected]>

Add support for the _POSIX_REALTIME_SIGNALS Option. This adds support for the functions sigqueue(), sigtimedwait(), and sigwaitinfo(). Signed-off-by: Chris Friedt <[email protected]>

Add tests for realtime signals. Note, that since the "is a" relationship between Zephyr and POSIX threads is not bidirectional, tests need to run inside of a POSIX thread in order to correctly translate a "pid_t" (which is necessarily an int due to it being defined as such in various C libraries) to a pthread_t, and subsequently to a k_tid_t. Of course, the kernel counterparts of the real-time signal API have no such restriction, as k_pid_t aliases k_tid_t. Signed-off-by: Chris Friedt <[email protected]>

Mark the POSIX_REALTIME_SIGNALS Option Group (and Option) as supported in Zephyr. This and POSIX_THREAD_SAFE_FUNCTIONS (as well as the other PRs for POSIX_DEVICE_IO, POSIX_FD_MGMT, and POSIX_SIGNALS) should make us functionally complete for the required system interfaces as well as PSE51. Signed-off-by: Chris Friedt <[email protected]>

Now that POSIX_REALTIME_SIGNALS is implemented, add the final piece of the POSIX_THREADS puzzle. Namely, pthread_kill(). This was the last function required to mark the POSIX_THREADS_BASE Option Group as fully supported in Zephyr. Additionally, we may now mark the _POSIX_THREADS Option as supported in Zephyr. The _POSIX_THREADS Option is required as part of the POSIX System Interfaces (Base Definitions) as well as PSE51, PSE52, PSE53, PSE54 and all other conformant POSIX subprofiles. Signed-off-by: Chris Friedt <[email protected]>

Test for pthread_kill() in signal.h Signed-off-by: Chris Friedt <[email protected]>

Mark the POSIX_THREADS_BASE Option Group as complete and mark the _POSIX_THREADS Option as supported. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2024-08-11T11:29:38Z

@jukkar, @ycsin - please revisit when you have a moment.
@kartben - @headerfile isn't used anywhere else in Zephyr currently, so I would prefer to not introduce it. The #include ... bit seems to be generated by SHOW_HEADERFILE

ycsin

I have mixed feelings over the intro/use of k_pid_t in the Zephyr APIs since the kernel actually do not support processes and not sure if it ever will, so instead of having this 'catch' in the Zephyr kernel layer, it might also make sense for us to simply treat thread as thread in Zephyr APIs and simply address/mention this limitation in the POSIX layer, but otherwise LGTM

cfriedt · 2024-08-20T15:29:25Z

@peter-mitsis , @andyross - would you be able to review this from the kernel side?

peter-mitsis

Years ago, Zephyr had a signal-like feature called alerts; however, this was removed in 2018 as no one was using it. This leads me to wonder, would it be better to have this entirely defined within the posix subsystem/library and not in the kernel?

peter-mitsis · 2024-08-21T18:03:02Z

kernel/signal.c

+#include <zephyr/sys/ring_buffer.h>
+
+#ifndef BITS_PER_BYTE
+#define BITS_PER_BYTE 8


I am surprised that this is not already in something like include/zephyr/sys/util.h

Yeah, same. I could factor that out into a separate commit or PR, if that works.

Made #79479

peter-mitsis · 2024-08-21T18:31:33Z

kernel/signal.c

+		return false;
+	}
+
+	if (_current == tid) {


I think that (_current == tid) can only be a valid check if done at thread level. If done at a ISR-level, a previous operation might have aborted it, but _current will have remained unchanged until execution leaves the interrupt.

Thanks for the insight - what is the best way to get the thread associated with a given syscall behind the syscall? k_current_get()?

If I am following the intent of the logic correctly, all we really need for this fast path check is

if ((!arch_is_in_isr() && (_current == tid)) { return true; }

Thus, if the operation was called by a thread, it is safe to test _current against tid. If it was invoked by an ISR, we fall through to the next check just as if the routine was called by a thread other than itself.

peter-mitsis · 2024-08-21T18:43:44Z

kernel/signal.c

+	if ((tid->base.thread_state & valid_states) != 0) {
+		return true;
+	}
+


I would feel a little more comfortable if the valid states were testing for invalid states rather than assuming that if a valid state bit is set, then all is ok.

That works - I'll make those changes.

peter-mitsis · 2024-08-21T18:44:06Z

kernel/signal.c

+static bool k_sig_pid_is_valid(k_pid_t pid)
+{
+	k_tid_t tid = (k_tid_t)pid;
+	const uint32_t valid_states = _THREAD_PENDING | _THREAD_SUSPENDED | _THREAD_QUEUED;


What about _THREAD_SUSPENDING?

peter-mitsis · 2024-08-21T19:35:15Z

kernel/signal.c

+
+static void k_sig_pending_unlocked(struct k_sig_set *set)
+{
+	struct k_thread *tid = _current;


Is this supposed to be callable from within an ISR? Again, I am seeing a potential issue with confusing the thread with the interrupted thread if called from an ISR. (There may be other cases where this arises)

This is really just a private function for use locally when the spinlock is already locked. It could probably be marked inline, but does not need to be inlined. I don't believe it would ever be called from an ISR context.

What is the best call to make to determine the thread ID associated with a syscall while the syscall is executing? k_current_get()?

When it comes to getting the current thread ID, _current can generally be used by kernel internals--I usually think of k_current_get() as being more application oriented.

I may have misread something, but it does look to me like this routine is ISR callable. It can be called by z_impl_k_sig_queue(), which in turn has documentation that explicitly states that it is safe to call in an ISR context. Given how it gets used, I suspect that all we would need is to make the k_sig_addset() also conditional upon the context not being an ISR.

if ((entry->tid == tid) && !arch_is_in_isr()) { k_sig_addset(set, entry->signo); }

It might be possible to call this with tid as a parameter, which, I think would solidify the use.

It is static here, so use of this function should be from behind a syscall, and not an ISR.

peter-mitsis · 2024-08-21T19:37:21Z

kernel/signal.c

+	}
+}
+
+static inline void k_sig_dump_set(const struct k_sig_set *set, const char *label)


Potential naming inconsistency: I observe that k_sig_fifo_dump() puts the verb 'dump' at the end whereas k_sig_dump_set() puts the verb in the second-to-last position.

Yes - thanks. I'll fix that.

nashif · 2024-08-21T20:40:01Z

Years ago, Zephyr had a signal-like feature called alerts; however, this was removed in 2018 as no one was using it. This leads me to wonder, would it be better to have this entirely defined within the posix subsystem/library and not in the kernel?

for reference: https://docs.zephyrproject.org/1.13.0/kernel/synchronization/alerts.html?highlight=alerts

cfriedt · 2024-08-21T23:53:25Z

would it be better to have this entirely defined within the posix subsystem/library and not in the kernel?

The primary reason is that, eventually, the same signal mechanism can be used by both libc and POSIX to signal threads (so that libc signals can be independent of POSIX).

The second reason is that (currently) Zephyr threads are not POSIX threads, so if it were to be only available at the POSIX thread level, then there would be an asymmetric way to signal. This simplifies and future-proofs signalling so it is bidirectional.

peter-mitsis · 2024-09-18T19:31:50Z

@cfriedt - Not sure if there is anything you need from me at the moment on this PR, but I thought I would ping you just in case I have overlooked something.

cfriedt · 2024-09-18T19:52:04Z

@peter-mitsis - I have to respin. Just in Vienna at LPC this week. I'll make sure to check about ISR vs syscall context.

cfriedt · 2024-10-13T11:57:30Z

I'd like to get #79479 in before this (to factor out the BITS_PER_BYTE macro). I have respun with changes requested, but will push after 79479 is in.

kartben · 2024-11-27T13:16:00Z

@cfriedt this needs rebasing

cfriedt requested review from ycsin, ceolin and jukkar June 25, 2024 02:47

cfriedt force-pushed the posix-realtime-signals2 branch 12 times, most recently from 431b3cd to 9f9125e Compare June 25, 2024 14:35

cfriedt force-pushed the posix-realtime-signals2 branch 4 times, most recently from f44d683 to a503415 Compare June 25, 2024 15:35

cfriedt mentioned this pull request Jun 25, 2024

POSIX Roadmap for LTSv3 #51211

Open

cfriedt marked this pull request as ready for review June 25, 2024 18:08

zephyrbot added area: POSIX POSIX API Library area: Kernel area: Toolchains Toolchains area: Base OS Base OS Library (lib/os) labels Jun 25, 2024

zephyrbot requested review from andyross, dcpleung, nashif and npitre June 25, 2024 18:09

cfriedt force-pushed the posix-realtime-signals2 branch from 2badd25 to dcaf32d Compare August 10, 2024 03:05

cfriedt force-pushed the posix-realtime-signals2 branch from dcaf32d to 624b350 Compare August 10, 2024 03:11

Chris Friedt added 10 commits August 10, 2024 09:08

ring_buffer: support static rings

cfb3ff9

Move initializers to a common macro and provide static variants. Signed-off-by: Chris Friedt <[email protected]>

kernel: initial support for signals

b2ea90e

This changes adds initial support for signals. Note: this feature is experimental. The primary use-case is intended to be real-time signals. Currently, there is no asynchronous signal delivery. Signed-off-by: Chris Friedt <[email protected]>

tests: kernel: add tests for k_sig_queue() and k_sig_timedwait()

3c0ec66

Add tests for realtime signals. Signed-off-by: Chris Friedt <[email protected]>

posix: realtime signals: add realtime signal support

4f4558b

Add support for the _POSIX_REALTIME_SIGNALS Option. This adds support for the functions sigqueue(), sigtimedwait(), and sigwaitinfo(). Signed-off-by: Chris Friedt <[email protected]>

tests: posix: headers: test for pthread_kill()

3dfe34c

Test for pthread_kill() in signal.h Signed-off-by: Chris Friedt <[email protected]>

doc: posix: mark posix threads as supported

4787f31

Mark the POSIX_THREADS_BASE Option Group as complete and mark the _POSIX_THREADS Option as supported. Signed-off-by: Chris Friedt <[email protected]>

cfriedt force-pushed the posix-realtime-signals2 branch from 624b350 to 4787f31 Compare August 10, 2024 13:08

ycsin approved these changes Aug 12, 2024

View reviewed changes

peter-mitsis reviewed Aug 21, 2024

View reviewed changes

cfriedt requested a review from peter-mitsis September 2, 2024 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posix + kernel: support for realtime signals #74918

posix + kernel: support for realtime signals #74918

cfriedt commented Jun 25, 2024 •

edited

Loading

cfriedt commented Jun 25, 2024 •

edited

Loading

cfriedt commented Aug 10, 2024

cfriedt commented Aug 10, 2024

cfriedt commented Aug 10, 2024 •

edited

Loading

kartben commented Aug 10, 2024

cfriedt commented Aug 10, 2024

kartben commented Aug 10, 2024

cfriedt commented Aug 11, 2024

ycsin left a comment •

edited

Loading

cfriedt commented Aug 20, 2024

peter-mitsis left a comment

peter-mitsis Aug 21, 2024

cfriedt Sep 2, 2024 •

edited

Loading

cfriedt Oct 6, 2024 •

edited

Loading

peter-mitsis Aug 21, 2024

cfriedt Sep 2, 2024 •

edited

Loading

peter-mitsis Sep 5, 2024

peter-mitsis Aug 21, 2024

cfriedt Sep 2, 2024

peter-mitsis Aug 21, 2024

peter-mitsis Aug 21, 2024

cfriedt Sep 2, 2024 •

edited

Loading

peter-mitsis Sep 5, 2024

cfriedt Sep 5, 2024

peter-mitsis Aug 21, 2024

cfriedt Sep 2, 2024

nashif commented Aug 21, 2024

cfriedt commented Aug 21, 2024

peter-mitsis commented Sep 18, 2024

cfriedt commented Sep 18, 2024

cfriedt commented Oct 13, 2024

kartben commented Nov 27, 2024

posix + kernel: support for realtime signals #74918

Are you sure you want to change the base?

posix + kernel: support for realtime signals #74918

Conversation

cfriedt commented Jun 25, 2024 • edited Loading

cfriedt commented Jun 25, 2024 • edited Loading

cfriedt commented Aug 10, 2024

cfriedt commented Aug 10, 2024

cfriedt commented Aug 10, 2024 • edited Loading

kartben commented Aug 10, 2024

cfriedt commented Aug 10, 2024

kartben commented Aug 10, 2024

cfriedt commented Aug 11, 2024

ycsin left a comment • edited Loading

Choose a reason for hiding this comment

cfriedt commented Aug 20, 2024

peter-mitsis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashif commented Aug 21, 2024

cfriedt commented Aug 21, 2024

peter-mitsis commented Sep 18, 2024

cfriedt commented Sep 18, 2024

cfriedt commented Oct 13, 2024

kartben commented Nov 27, 2024

cfriedt commented Jun 25, 2024 •

edited

Loading

cfriedt commented Jun 25, 2024 •

edited

Loading

cfriedt commented Aug 10, 2024 •

edited

Loading

ycsin left a comment •

edited

Loading

cfriedt Sep 2, 2024 •

edited

Loading

cfriedt Oct 6, 2024 •

edited

Loading

cfriedt Sep 2, 2024 •

edited

Loading

cfriedt Sep 2, 2024 •

edited

Loading