From 95f1ee235ae257802a94d7e94d476ea0aaea5cd8 Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Tue, 13 Jan 2015 19:53:15 -0800 Subject: [PATCH] Change on handling of SIGEV_THREAD timers. 1. Don't prevent calling callback when SIGEV_THREAD timers are disarmed by timer_settime. As in POSIX standard: The effect of disarming or resetting a timer with pending expiration notifications is unspecified. And glibc didn't prevent in this situation, so I think it is fine to remove the support. 2. Still prevent calling callback when SIGEV_THREAD timers are deleted by timer_delete. As in POSIX standard: The disposition of pending signals for the deleted timer is unspecified. However, glibc handles this (although that is not perfect). And some of our tests in time_test.cpp depend on this feature as described in b/18039727. so I retain the support. 3. Fix some flaky test in time_test.cpp, and make "time*" test pass on bionic-unit-tests-glibcxx. Bug: 18263854 Change-Id: I8ced184eacdbfcf433fd81b0c69c38824beb8ebc --- libc/bionic/posix_timers.cpp | 42 +++++------ tests/time_test.cpp | 142 ++++++++++++++++++----------------- 2 files changed, 92 insertions(+), 92 deletions(-) diff --git a/libc/bionic/posix_timers.cpp b/libc/bionic/posix_timers.cpp index 999157371..bc3aeb21c 100644 --- a/libc/bionic/posix_timers.cpp +++ b/libc/bionic/posix_timers.cpp @@ -26,14 +26,15 @@ * SUCH DAMAGE. */ -#include "pthread_internal.h" -#include "private/bionic_futex.h" #include "private/kernel_sigset_t.h" #include #include +#include +#include #include #include +#include // System calls. extern "C" int __rt_sigtimedwait(const sigset_t*, siginfo_t*, const struct timespec*, size_t); @@ -59,11 +60,11 @@ struct PosixTimer { int sigev_notify; - // These fields are only needed for a SIGEV_THREAD timer. + // The fields below are only needed for a SIGEV_THREAD timer. pthread_t callback_thread; void (*callback)(sigval_t); sigval_t callback_argument; - volatile bool armed; + atomic_bool deleted; // Set when the timer is deleted, to prevent further calling of callback. }; static __kernel_timer_t to_kernel_timer_id(timer_t timer) { @@ -85,8 +86,13 @@ static void* __timer_thread_start(void* arg) { continue; } - if (si.si_code == SI_TIMER && timer->armed) { + if (si.si_code == SI_TIMER) { // This signal was sent because a timer fired, so call the callback. + + // All events to the callback will be ignored when the timer is deleted. + if (atomic_load(&timer->deleted) == true) { + continue; + } timer->callback(timer->callback_argument); } else if (si.si_code == SI_TKILL) { // This signal was sent because someone wants us to exit. @@ -97,9 +103,7 @@ static void* __timer_thread_start(void* arg) { } static void __timer_thread_stop(PosixTimer* timer) { - // Immediately mark the timer as disarmed so even if some events - // continue to happen, the callback won't be called. - timer->armed = false; + atomic_store(&timer->deleted, true); pthread_kill(timer->callback_thread, TIMER_SIGNAL); } @@ -126,7 +130,7 @@ int timer_create(clockid_t clock_id, sigevent* evp, timer_t* timer_id) { // Otherwise, this must be SIGEV_THREAD timer... timer->callback = evp->sigev_notify_function; timer->callback_argument = evp->sigev_value; - timer->armed = false; + atomic_init(&timer->deleted, false); // Check arguments that the kernel doesn't care about but we do. if (timer->callback == NULL) { @@ -199,25 +203,19 @@ int timer_delete(timer_t id) { return 0; } -// http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html +// http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_gettime.html int timer_gettime(timer_t id, itimerspec* ts) { return __timer_gettime(to_kernel_timer_id(id), ts); } -// http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html +// http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_settime.html +// When using timer_settime to disarm a repeatable SIGEV_THREAD timer with a very small +// period (like below 1ms), the kernel may continue to send events to the callback thread +// for a few extra times. This behavior is fine because in POSIX standard: The effect of +// disarming or resetting a timer with pending expiration notifications is unspecified. int timer_settime(timer_t id, int flags, const itimerspec* ts, itimerspec* ots) { PosixTimer* timer= reinterpret_cast(id); - int rc = __timer_settime(timer->kernel_timer_id, flags, ts, ots); - if (rc == 0) { - // Mark the timer as either being armed or disarmed. This avoids the - // callback being called after the disarm for SIGEV_THREAD timers only. - if (ts->it_value.tv_sec != 0 || ts->it_value.tv_nsec != 0) { - timer->armed = true; - } else { - timer->armed = false; - } - } - return rc; + return __timer_settime(timer->kernel_timer_id, flags, ts, ots); } // http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html diff --git a/tests/time_test.cpp b/tests/time_test.cpp index 691d8ff9c..a0b0209ab 100644 --- a/tests/time_test.cpp +++ b/tests/time_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include "ScopedSignalHandler.h" @@ -197,7 +198,7 @@ TEST(time, timer_create) { ASSERT_EQ(0, timer_delete(timer_id)); } -static int timer_create_SIGEV_SIGNAL_signal_handler_invocation_count = 0; +static int timer_create_SIGEV_SIGNAL_signal_handler_invocation_count; static void timer_create_SIGEV_SIGNAL_signal_handler(int signal_number) { ++timer_create_SIGEV_SIGNAL_signal_handler_invocation_count; ASSERT_EQ(SIGUSR1, signal_number); @@ -212,6 +213,7 @@ TEST(time, timer_create_SIGEV_SIGNAL) { timer_t timer_id; ASSERT_EQ(0, timer_create(CLOCK_MONOTONIC, &se, &timer_id)); + timer_create_SIGEV_SIGNAL_signal_handler_invocation_count = 0; ScopedSignalHandler ssh(SIGUSR1, timer_create_SIGEV_SIGNAL_signal_handler); ASSERT_EQ(0, timer_create_SIGEV_SIGNAL_signal_handler_invocation_count); @@ -228,25 +230,26 @@ TEST(time, timer_create_SIGEV_SIGNAL) { } struct Counter { - volatile int value; + private: + std::atomic value; timer_t timer_id; sigevent_t se; bool timer_valid; - Counter(void (*fn)(sigval_t)) : value(0), timer_valid(false) { - memset(&se, 0, sizeof(se)); - se.sigev_notify = SIGEV_THREAD; - se.sigev_notify_function = fn; - se.sigev_value.sival_ptr = this; - Create(); - } - void Create() { ASSERT_FALSE(timer_valid); ASSERT_EQ(0, timer_create(CLOCK_REALTIME, &se, &timer_id)); timer_valid = true; } + public: + Counter(void (*fn)(sigval_t)) : value(0), timer_valid(false) { + memset(&se, 0, sizeof(se)); + se.sigev_notify = SIGEV_THREAD; + se.sigev_notify_function = fn; + se.sigev_value.sival_ptr = this; + Create(); + } void DeleteTimer() { ASSERT_TRUE(timer_valid); ASSERT_EQ(0, timer_delete(timer_id)); @@ -259,12 +262,16 @@ struct Counter { } } + int Value() const { + return value; + } + void SetTime(time_t value_s, time_t value_ns, time_t interval_s, time_t interval_ns) { ::SetTime(timer_id, value_s, value_ns, interval_s, interval_ns); } bool ValueUpdated() { - volatile int current_value = value; + int current_value = value; time_t start = time(NULL); while (current_value == value && (time(NULL) - start) < 5) { } @@ -287,30 +294,29 @@ struct Counter { TEST(time, timer_settime_0) { Counter counter(Counter::CountAndDisarmNotifyFunction); - ASSERT_TRUE(counter.timer_valid); - - ASSERT_EQ(0, counter.value); + ASSERT_EQ(0, counter.Value()); counter.SetTime(0, 1, 1, 0); usleep(500000); // The count should just be 1 because we disarmed the timer the first time it fired. - ASSERT_EQ(1, counter.value); + ASSERT_EQ(1, counter.Value()); } TEST(time, timer_settime_repeats) { Counter counter(Counter::CountNotifyFunction); - ASSERT_TRUE(counter.timer_valid); - - ASSERT_EQ(0, counter.value); + ASSERT_EQ(0, counter.Value()); counter.SetTime(0, 1, 0, 10); ASSERT_TRUE(counter.ValueUpdated()); ASSERT_TRUE(counter.ValueUpdated()); ASSERT_TRUE(counter.ValueUpdated()); + counter.DeleteTimer(); + // Add a sleep as other threads may be calling the callback function when the timer is deleted. + usleep(500000); } -static int timer_create_NULL_signal_handler_invocation_count = 0; +static int timer_create_NULL_signal_handler_invocation_count; static void timer_create_NULL_signal_handler(int signal_number) { ++timer_create_NULL_signal_handler_invocation_count; ASSERT_EQ(SIGALRM, signal_number); @@ -321,6 +327,7 @@ TEST(time, timer_create_NULL) { timer_t timer_id; ASSERT_EQ(0, timer_create(CLOCK_MONOTONIC, NULL, &timer_id)); + timer_create_NULL_signal_handler_invocation_count = 0; ScopedSignalHandler ssh(SIGALRM, timer_create_NULL_signal_handler); ASSERT_EQ(0, timer_create_NULL_signal_handler_invocation_count); @@ -367,22 +374,59 @@ TEST(time, timer_delete_multiple) { TEST(time, timer_create_multiple) { Counter counter1(Counter::CountNotifyFunction); - ASSERT_TRUE(counter1.timer_valid); Counter counter2(Counter::CountNotifyFunction); - ASSERT_TRUE(counter2.timer_valid); Counter counter3(Counter::CountNotifyFunction); - ASSERT_TRUE(counter3.timer_valid); - ASSERT_EQ(0, counter1.value); - ASSERT_EQ(0, counter2.value); - ASSERT_EQ(0, counter3.value); + ASSERT_EQ(0, counter1.Value()); + ASSERT_EQ(0, counter2.Value()); + ASSERT_EQ(0, counter3.Value()); counter2.SetTime(0, 1, 0, 0); usleep(500000); - EXPECT_EQ(0, counter1.value); - EXPECT_EQ(1, counter2.value); - EXPECT_EQ(0, counter3.value); + EXPECT_EQ(0, counter1.Value()); + EXPECT_EQ(1, counter2.Value()); + EXPECT_EQ(0, counter3.Value()); +} + +// Test to verify that disarming a repeatable timer disables the callbacks. +TEST(time, timer_disarm_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_EQ(0, counter.Value()); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.SetTime(0, 0, 0, 0); + // Add a sleep as the kernel may have pending events when the timer is disarmed. + usleep(500000); + int value = counter.Value(); + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.Value()); +} + +// Test to verify that deleting a repeatable timer disables the callbacks. +TEST(time, timer_delete_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_EQ(0, counter.Value()); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.DeleteTimer(); + // Add a sleep as other threads may be calling the callback function when the timer is deleted. + usleep(500000); + int value = counter.Value(); + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.Value()); } struct TimerDeleteData { @@ -499,45 +543,3 @@ TEST(time, clock_nanosleep) { timespec out; ASSERT_EQ(EINVAL, clock_nanosleep(-1, 0, &in, &out)); } - -// Test to verify that disarming a repeatable timer disables the -// callbacks. -TEST(time, timer_disarm_terminates) { - Counter counter(Counter::CountNotifyFunction); - ASSERT_TRUE(counter.timer_valid); - - ASSERT_EQ(0, counter.value); - - counter.SetTime(0, 1, 0, 1); - ASSERT_TRUE(counter.ValueUpdated()); - ASSERT_TRUE(counter.ValueUpdated()); - ASSERT_TRUE(counter.ValueUpdated()); - - counter.SetTime(0, 0, 1, 0); - volatile int value = counter.value; - usleep(500000); - - // Verify the counter has not been incremented. - ASSERT_EQ(value, counter.value); -} - -// Test to verify that deleting a repeatable timer disables the -// callbacks. -TEST(time, timer_delete_terminates) { - Counter counter(Counter::CountNotifyFunction); - ASSERT_TRUE(counter.timer_valid); - - ASSERT_EQ(0, counter.value); - - counter.SetTime(0, 1, 0, 1); - ASSERT_TRUE(counter.ValueUpdated()); - ASSERT_TRUE(counter.ValueUpdated()); - ASSERT_TRUE(counter.ValueUpdated()); - - counter.DeleteTimer(); - volatile int value = counter.value; - usleep(500000); - - // Verify the counter has not been incremented. - ASSERT_EQ(value, counter.value); -}