Skip to content

posix: timer: use async pthread cancellation #67871

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

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Jan 20, 2024

Previously, Zephyr's POSIX API did not differentiate between deferred and asynchronous pthread cancellation. In fact all pthread cancellation was asynchronous. According to the spec, all pthreads should be created with deferred cancellation by default.

Note

PTHREAD_CANCEL_ASYNCHRONOUS means cancel asynchronously with respect to cancellation points (synchronously with respect to pthread_cancel(), which is perhaps unintuitive)

The POSIX timer relied on this non-standard convention.

Oddly, this change prevents what would have otherwise been a regression that would have been caused by fixing pthread behaviour (in #67223).

We are effectively uncovering bugs which were probably always present in the pthread.c and timer.c implementations going back quite a few years.

Fixes #67870
Fixes #67874

Additionally:

  • remove non-conformant error condition when using a clock other than CLOCK_MONOTONIC
  • improve struct sigevent and union sigval alignment / padding

The sigevent struct and sigval union members were previously not
ordered in a way that produces optimal alignment / reduces
padding on 64-bit systems.

Reorder members so that pointers come first.

Signed-off-by: Christopher Friedt <[email protected]>
There is no requirement that says e.g. CLOCK_REALTIME cannot be
used for timer_create(). In fact, the spec explicitly requires
it. It might not be ideal, but users should still be able to
use it.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt requested review from jukkar and ycsin January 20, 2024 14:54
@cfriedt cfriedt force-pushed the fix-timer-overrun-issue branch 2 times, most recently from df02a9b to 701e7db Compare January 20, 2024 15:00
Previously, Zephyr's POSIX API did not differentiate between
deferred and asynchronous pthread cancellation. In fact all
pthread cancellation was asynchronous. According to the spec,
all pthreads should be created with deferred cancellation by
default.

Note: PTHREAD_CANCEL_ASYNCHRONOUS means cancel asynchronously
with respect to cancellation points (but synchronously with
respect to the thread that callse pthread_cancel(), which is
perhaps unintuitive).

The POSIX timer relied on this non-standard convention.

Oddly, this change prevents what would have otherwise been a
regression that would have been caused by fixing pthread
behaviour (in a separate commit).

We are effectively uncovering bugs which were probably always
present in the pthread.c and timer.c implementations going
back quite a few years.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt force-pushed the fix-timer-overrun-issue branch from 701e7db to a490081 Compare January 20, 2024 15:07
@cfriedt cfriedt changed the title Fix timer overrun issue posix: timer: use async pthread cancellation Jan 20, 2024
@cfriedt cfriedt marked this pull request as ready for review January 20, 2024 15:14
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Jan 20, 2024
@cfriedt cfriedt added the bug The issue is a bug, or the PR is fixing a bug label Jan 20, 2024
Ensure that the realtime clock may also be used with
timer_create().

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
Copy link
Member Author

cfriedt commented Jan 21, 2024

This is blocking #67223

@cfriedt cfriedt merged commit 7f57d5d into zephyrproject-rtos:main Jan 22, 2024
@cfriedt cfriedt deleted the fix-timer-overrun-issue branch January 22, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
4 participants