Date: Tue, 7 Aug 2018 14:39:00 +0000 (UTC) From: David Bright <dab@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r337418 - in stable/11: lib/libc/sys sys/kern tests/sys/kqueue/libkqueue Message-ID: <201808071439.w77Ed0Q3064840@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: dab Date: Tue Aug 7 14:39:00 2018 New Revision: 337418 URL: https://svnweb.freebsd.org/changeset/base/337418 Log: MFC r336761 & r336781: Allow a EVFILT_TIMER kevent to be updated. If a timer is updated (re-added) with a different time period (specified in the .data field of the kevent), the new time period has no effect; the timer will not expire until the original time has elapsed. This violates the documented behavior as the kqueue(2) man page says (in part) "Re-adding an existing event will modify the parameters of the original event, and not result in a duplicate entry." This modification, adapted from a patch submitted by cem@ to PR214987, fixes the kqueue system to allow updating a timer entry. The kevent timer behavior is changed to: * When a timer is re-added, update the timer parameters to and re-start the timer using the new parameters. * Allow updating both active and already expired timers. * When the timer has already expired, dequeue any undelivered events and clear the count of expirations. All of these changes address the original PR and also bring the FreeBSD and macOS kevent timer behaviors into agreement. A few other changes were made along the way: * Update the kqueue(2) man page to reflect the new timer behavior. * Fix man page style issues in kqueue(2) diagnosed by igor. * Update the timer libkqueue system test to test for the updated timer behavior. * Fix the (test) libkqueue common.h file so that it includes config.h which defines various HAVE_* feature defines, before the #if tests for such variables in common.h. This enables the use of the actual err(3) family of functions. * Fix the usages of the err(3) functions in the tests for incorrect type of variables. Those were formerly undiagnosed due to the disablement of the err(3) functions (see previous bullet point). PR: 214987 Relnotes: yes Sponsored by: Dell EMC Modified: stable/11/lib/libc/sys/kqueue.2 stable/11/sys/kern/kern_event.c stable/11/tests/sys/kqueue/libkqueue/common.h stable/11/tests/sys/kqueue/libkqueue/main.c stable/11/tests/sys/kqueue/libkqueue/timer.c Directory Properties: stable/11/ (props changed) Modified: stable/11/lib/libc/sys/kqueue.2 ============================================================================== --- stable/11/lib/libc/sys/kqueue.2 Tue Aug 7 14:09:29 2018 (r337417) +++ stable/11/lib/libc/sys/kqueue.2 Tue Aug 7 14:39:00 2018 (r337418) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd June 22, 2017 +.Dd July 27, 2018 .Dt KQUEUE 2 .Os .Sh NAME @@ -339,8 +339,8 @@ Events for this filter are not registered with .Fn kevent directly but are registered via the .Va aio_sigevent -member of an asychronous I/O request when it is scheduled via an asychronous I/O -system call such as +member of an asynchronous I/O request when it is scheduled via an +asynchronous I/O system call such as .Fn aio_read . The filter returns under the same conditions as .Fn aio_error . @@ -485,7 +485,7 @@ been marked as .Dv SIG_IGN , except for the .Dv SIGCHLD -signal, which, if ignored, won't be recorded by the filter. +signal, which, if ignored, will not be recorded by the filter. Event notification happens after normal signal delivery processing. .Va data @@ -530,6 +530,13 @@ On return, .Va fflags contains the events which triggered the filter. .Pp +If an existing timer is re-added, the existing timer will be +effectively canceled (throwing away any undelivered record of previous +timer expiration) and re-started using the new parameters contained in +.Va data +and +.Va fflags . +.Pp There is a system wide limit on the number of timers which is controlled by the .Va kern.kq_calloutmax @@ -576,9 +583,9 @@ contains the users defined flags in the lower 24 bits. .Sh CANCELLATION BEHAVIOUR If .Fa nevents -is non-zero, i.e. the function is potentially blocking, the call +is non-zero, i.e., the function is potentially blocking, the call is a cancellation point. -Otherwise, i.e. if +Otherwise, i.e., if .Fa nevents is zero, the call is not cancellable. Cancellation can only occur before any changes are made to the kqueue, Modified: stable/11/sys/kern/kern_event.c ============================================================================== --- stable/11/sys/kern/kern_event.c Tue Aug 7 14:09:29 2018 (r337417) +++ stable/11/sys/kern/kern_event.c Tue Aug 7 14:39:00 2018 (r337418) @@ -151,6 +151,10 @@ static int filt_fileattach(struct knote *kn); static void filt_timerexpire(void *knx); static int filt_timerattach(struct knote *kn); static void filt_timerdetach(struct knote *kn); +static void filt_timerstart(struct knote *kn, sbintime_t to); +static void filt_timertouch(struct knote *kn, struct kevent *kev, + u_long type); +static int filt_timervalidate(struct knote *kn, sbintime_t *to); static int filt_timer(struct knote *kn, long hint); static int filt_userattach(struct knote *kn); static void filt_userdetach(struct knote *kn); @@ -179,6 +183,7 @@ static struct filterops timer_filtops = { .f_attach = filt_timerattach, .f_detach = filt_timerdetach, .f_event = filt_timer, + .f_touch = filt_timertouch, }; static struct filterops user_filtops = { .f_attach = filt_userattach, @@ -661,24 +666,38 @@ filt_timerexpire(void *knx) * data contains amount of time to sleep */ static int -filt_timerattach(struct knote *kn) +filt_timervalidate(struct knote *kn, sbintime_t *to) { - struct kq_timer_cb_data *kc; - sbintime_t to; - unsigned int ncallouts; if (kn->kn_sdata < 0) return (EINVAL); if (kn->kn_sdata == 0 && (kn->kn_flags & EV_ONESHOT) == 0) kn->kn_sdata = 1; - /* Only precision unit are supported in flags so far */ + /* + * The only fflags values supported are the timer unit + * (precision) and the absolute time indicator. + */ if ((kn->kn_sfflags & ~NOTE_TIMER_PRECMASK) != 0) return (EINVAL); - to = timer2sbintime(kn->kn_sdata, kn->kn_sfflags); - if (to < 0) + *to = timer2sbintime(kn->kn_sdata, kn->kn_sfflags); + if (*to < 0) return (EINVAL); + return (0); +} +static int +filt_timerattach(struct knote *kn) +{ + struct kq_timer_cb_data *kc; + sbintime_t to; + unsigned int ncallouts; + int error; + + error = filt_timervalidate(kn, &to); + if (error != 0) + return (error); + do { ncallouts = kq_ncallouts; if (ncallouts >= kq_calloutmax) @@ -689,12 +708,21 @@ filt_timerattach(struct knote *kn) kn->kn_status &= ~KN_DETACHED; /* knlist_add clears it */ kn->kn_ptr.p_v = kc = malloc(sizeof(*kc), M_KQUEUE, M_WAITOK); callout_init(&kc->c, 1); + filt_timerstart(kn, to); + + return (0); +} + +static void +filt_timerstart(struct knote *kn, sbintime_t to) +{ + struct kq_timer_cb_data *kc; + + kc = kn->kn_ptr.p_v; kc->next = to + sbinuptime(); kc->to = to; callout_reset_sbt_on(&kc->c, kc->next, 0, filt_timerexpire, kn, PCPU_GET(cpuid), C_ABSOLUTE); - - return (0); } static void @@ -709,6 +737,73 @@ filt_timerdetach(struct knote *kn) old = atomic_fetchadd_int(&kq_ncallouts, -1); KASSERT(old > 0, ("Number of callouts cannot become negative")); kn->kn_status |= KN_DETACHED; /* knlist_remove sets it */ +} + +static void +filt_timertouch(struct knote *kn, struct kevent *kev, u_long type) +{ + struct kq_timer_cb_data *kc; + struct kqueue *kq; + sbintime_t to; + int error; + + switch (type) { + case EVENT_REGISTER: + /* Handle re-added timers that update data/fflags */ + if (kev->flags & EV_ADD) { + kc = kn->kn_ptr.p_v; + + /* Drain any existing callout. */ + callout_drain(&kc->c); + + /* Throw away any existing undelivered record + * of the timer expiration. This is done under + * the presumption that if a process is + * re-adding this timer with new parameters, + * it is no longer interested in what may have + * happened under the old parameters. If it is + * interested, it can wait for the expiration, + * delete the old timer definition, and then + * add the new one. + * + * This has to be done while the kq is locked: + * - if enqueued, dequeue + * - make it no longer active + * - clear the count of expiration events + */ + kq = kn->kn_kq; + KQ_LOCK(kq); + if (kn->kn_status & KN_QUEUED) + knote_dequeue(kn); + + kn->kn_status &= ~KN_ACTIVE; + kn->kn_data = 0; + KQ_UNLOCK(kq); + + /* Reschedule timer based on new data/fflags */ + kn->kn_sfflags = kev->fflags; + kn->kn_sdata = kev->data; + error = filt_timervalidate(kn, &to); + if (error != 0) { + kn->kn_flags |= EV_ERROR; + kn->kn_data = error; + } else + filt_timerstart(kn, to); + } + break; + + case EVENT_PROCESS: + *kev = kn->kn_kevent; + if (kn->kn_flags & EV_CLEAR) { + kn->kn_data = 0; + kn->kn_fflags = 0; + } + break; + + default: + panic("filt_timertouch() - invalid type (%ld)", type); + break; + } } static int Modified: stable/11/tests/sys/kqueue/libkqueue/common.h ============================================================================== --- stable/11/tests/sys/kqueue/libkqueue/common.h Tue Aug 7 14:09:29 2018 (r337417) +++ stable/11/tests/sys/kqueue/libkqueue/common.h Tue Aug 7 14:39:00 2018 (r337418) @@ -19,6 +19,7 @@ #ifndef _COMMON_H #define _COMMON_H +#include "config.h" /* Needed for HAVE_* defines */ #if HAVE_ERR_H # include <err.h> @@ -39,8 +40,6 @@ #include <sys/event.h> -#include "config.h" - extern char *cur_test_id; int vnode_fd; @@ -72,6 +71,7 @@ kevent_add(int kqfd, struct kevent *kev, /* Checks if any events are pending, which is an error. */ extern void test_no_kevents(void); +extern void test_no_kevents_quietly(void); extern void test_begin(const char *); extern void success(void); Modified: stable/11/tests/sys/kqueue/libkqueue/main.c ============================================================================== --- stable/11/tests/sys/kqueue/libkqueue/main.c Tue Aug 7 14:09:29 2018 (r337417) +++ stable/11/tests/sys/kqueue/libkqueue/main.c Tue Aug 7 14:39:00 2018 (r337418) @@ -52,6 +52,25 @@ test_no_kevents(void) } } +/* Checks if any events are pending, which is an error. Do not print + * out anything unless events are found. +*/ +void +test_no_kevents_quietly(void) +{ + int nfds; + struct timespec timeo; + struct kevent kev; + + memset(&timeo, 0, sizeof(timeo)); + nfds = kevent(kqfd, NULL, 0, &kev, 1, &timeo); + if (nfds != 0) { + puts("\nUnexpected event:"); + puts(kevent_to_str(&kev)); + errx(1, "%d event(s) pending, but none expected:", nfds); + } +} + /* Retrieve a single kevent */ struct kevent * kevent_get(int kqfd) Modified: stable/11/tests/sys/kqueue/libkqueue/timer.c ============================================================================== --- stable/11/tests/sys/kqueue/libkqueue/timer.c Tue Aug 7 14:09:29 2018 (r337417) +++ stable/11/tests/sys/kqueue/libkqueue/timer.c Tue Aug 7 14:39:00 2018 (r337418) @@ -17,10 +17,61 @@ */ #include "common.h" +#include <sys/time.h> +#define MILLION 1000000 +#define THOUSAND 1000 +#define SEC_TO_MS(t) ((t) * THOUSAND) /* Convert seconds to milliseconds. */ +#define SEC_TO_US(t) ((t) * MILLION) /* Convert seconds to microseconds. */ +#define MS_TO_US(t) ((t) * THOUSAND) /* Convert milliseconds to microseconds. */ +#define US_TO_NS(t) ((t) * THOUSAND) /* Convert microseconds to nanoseconds. */ + int kqfd; +/* Get the current time with microsecond precision. Used for + * sub-second timing to make some timer tests run faster. + */ +static long +now(void) +{ + + struct timeval tv; + + gettimeofday(&tv, NULL); + return SEC_TO_US(tv.tv_sec) + tv.tv_usec; +} + +/* Sleep for a given number of milliseconds. The timeout is assumed to + * be less than 1 second. + */ void +mssleep(int t) +{ + + struct timespec stime = { + .tv_sec = 0, + .tv_nsec = US_TO_NS(MS_TO_US(t)), + }; + + nanosleep(&stime, NULL); +} + +/* Sleep for a given number of microseconds. The timeout is assumed to + * be less than 1 second. + */ +void +ussleep(int t) +{ + + struct timespec stime = { + .tv_sec = 0, + .tv_nsec = US_TO_NS(t), + }; + + nanosleep(&stime, NULL); +} + +void test_kevent_timer_add(void) { const char *test_id = "kevent(EVFILT_TIMER, EV_ADD)"; @@ -164,15 +215,282 @@ disable_and_enable(void) success(); } +static void +test_update(void) +{ + const char *test_id = "kevent(EVFILT_TIMER (UPDATE), EV_ADD | EV_ONESHOT)"; + struct kevent kev; + long elapsed; + long start; + + test_begin(test_id); + + test_no_kevents(); + + /* First set the timer to 1 second */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD | EV_ONESHOT, + NOTE_USECONDS, SEC_TO_US(1), (void *)1); + start = now(); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Now reduce the timer to 1 ms */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD | EV_ONESHOT, + NOTE_USECONDS, MS_TO_US(1), (void *)2); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Wait for the event */ + kev.flags |= EV_CLEAR; + kev.fflags &= ~NOTE_USECONDS; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + elapsed = now() - start; + + /* Check that the timer expired after at least 1 ms, but less than + * 1 second. This check is to make sure that the original 1 second + * timeout was not used. + */ + printf("timer expired after %ld us\n", elapsed); + if (elapsed < MS_TO_US(1)) + errx(1, "early timer expiration: %ld us", elapsed); + if (elapsed > SEC_TO_US(1)) + errx(1, "late timer expiration: %ld us", elapsed); + + success(); +} + +static void +test_update_equal(void) +{ + const char *test_id = "kevent(EVFILT_TIMER (UPDATE=), EV_ADD | EV_ONESHOT)"; + struct kevent kev; + long elapsed; + long start; + + test_begin(test_id); + + test_no_kevents(); + + /* First set the timer to 1 ms */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD | EV_ONESHOT, + NOTE_USECONDS, MS_TO_US(1), NULL); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Sleep for a significant fraction of the timeout. */ + ussleep(600); + + /* Now re-add the timer with the same parameters */ + start = now(); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Wait for the event */ + kev.flags |= EV_CLEAR; + kev.fflags &= ~NOTE_USECONDS; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + elapsed = now() - start; + + /* Check that the timer expired after at least 1 ms. This check is + * to make sure that the timer re-started and that the event is + * not from the original add of the timer. + */ + printf("timer expired after %ld us\n", elapsed); + if (elapsed < MS_TO_US(1)) + errx(1, "early timer expiration: %ld us", elapsed); + + success(); +} + +static void +test_update_expired(void) +{ + const char *test_id = "kevent(EVFILT_TIMER (UPDATE EXP), EV_ADD | EV_ONESHOT)"; + struct kevent kev; + long elapsed; + long start; + + test_begin(test_id); + + test_no_kevents(); + + /* Set the timer to 1ms */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD | EV_ONESHOT, + NOTE_USECONDS, MS_TO_US(1), NULL); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Wait for 2 ms to give the timer plenty of time to expire. */ + mssleep(2); + + /* Now re-add the timer */ + start = now(); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Wait for the event */ + kev.flags |= EV_CLEAR; + kev.fflags &= ~NOTE_USECONDS; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + elapsed = now() - start; + + /* Check that the timer expired after at least 1 ms. This check + * is to make sure that the timer re-started and that the event is + * not from the original add (and expiration) of the timer. + */ + printf("timer expired after %ld us\n", elapsed); + if (elapsed < MS_TO_US(1)) + errx(1, "early timer expiration: %ld us", elapsed); + + /* Make sure the re-added timer does not fire. In other words, + * test that the event received above was the only event from the + * add and re-add of the timer. + */ + mssleep(2); + test_no_kevents(); + + success(); +} + +static void +test_update_periodic(void) +{ + const char *test_id = "kevent(EVFILT_TIMER (UPDATE), periodic)"; + struct kevent kev; + long elapsed; + long start; + long stop; + + test_begin(test_id); + + test_no_kevents(); + + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD, 0, SEC_TO_MS(1), NULL); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Retrieve the event */ + kev.flags = EV_ADD | EV_CLEAR; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + + /* Check if the event occurs again */ + sleep(1); + kevent_cmp(&kev, kevent_get(kqfd)); + + /* Re-add with new timeout. */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD, 0, SEC_TO_MS(2), NULL); + start = now(); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Retrieve the event */ + kev.flags = EV_ADD | EV_CLEAR; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + + stop = now(); + elapsed = stop - start; + + /* Check that the timer expired after at least 2 ms. + */ + printf("timer expired after %ld us\n", elapsed); + if (elapsed < MS_TO_US(2)) + errx(1, "early timer expiration: %ld us", elapsed); + + /* Delete the event */ + kev.flags = EV_DELETE; + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + success(); +} + +static void +test_update_timing(void) +{ +#define MIN_SLEEP 500 +#define MAX_SLEEP 1500 + const char *test_id = "kevent(EVFILT_TIMER (UPDATE TIMING), EV_ADD | EV_ONESHOT)"; + struct kevent kev; + int iteration; + int sleeptime; + long elapsed; + long start; + long stop; + + test_begin(test_id); + + test_no_kevents(); + + /* Re-try the update tests with a variety of delays between the + * original timer activation and the update of the timer. The goal + * is to show that in all cases the only timer event that is + * received is from the update and not the original timer add. + */ + for (sleeptime = MIN_SLEEP, iteration = 1; + sleeptime < MAX_SLEEP; + ++sleeptime, ++iteration) { + + /* First set the timer to 1 ms */ + EV_SET(&kev, vnode_fd, EVFILT_TIMER, EV_ADD | EV_ONESHOT, + NOTE_USECONDS, MS_TO_US(1), NULL); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Delay; the delay ranges from less than to greater than the + * timer period. + */ + ussleep(sleeptime); + + /* Now re-add the timer with the same parameters */ + start = now(); + if (kevent(kqfd, &kev, 1, NULL, 0, NULL) < 0) + err(1, "%s", test_id); + + /* Wait for the event */ + kev.flags |= EV_CLEAR; + kev.fflags &= ~NOTE_USECONDS; + kev.data = 1; + kevent_cmp(&kev, kevent_get(kqfd)); + stop = now(); + elapsed = stop - start; + + /* Check that the timer expired after at least 1 ms. This + * check is to make sure that the timer re-started and that + * the event is not from the original add of the timer. + */ + if (elapsed < MS_TO_US(1)) + errx(1, "early timer expiration: %ld us", elapsed); + + /* Make sure the re-added timer does not fire. In other words, + * test that the event received above was the only event from + * the add and re-add of the timer. + */ + mssleep(2); + test_no_kevents_quietly(); + } + + success(); +} + void test_evfilt_timer() { kqfd = kqueue(); - test_kevent_timer_add(); - test_kevent_timer_del(); - test_kevent_timer_get(); - test_oneshot(); - test_periodic(); - disable_and_enable(); + test_kevent_timer_add(); + test_kevent_timer_del(); + test_kevent_timer_get(); + test_oneshot(); + test_periodic(); + test_update(); + test_update_equal(); + test_update_expired(); + test_update_timing(); + test_update_periodic(); + disable_and_enable(); close(kqfd); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808071439.w77Ed0Q3064840>