Date: Fri, 27 Jul 2018 13:49:17 +0000 (UTC) From: David Bright <dab@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r336761 - in head: lib/libc/sys sys/kern tests/sys/kqueue/libkqueue Message-ID: <201807271349.w6RDnHoX070437@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: dab Date: Fri Jul 27 13:49:17 2018 New Revision: 336761 URL: https://svnweb.freebsd.org/changeset/base/336761 Log: 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 Reported by: Brian Wellington <bwelling@xbill.org> Reviewed by: kib MFC after: 1 week Relnotes: yes Sponsored by: Dell EMC Differential Revision: https://reviews.freebsd.org/D15778 Modified: head/lib/libc/sys/kqueue.2 head/sys/kern/kern_event.c head/tests/sys/kqueue/libkqueue/common.h head/tests/sys/kqueue/libkqueue/main.c head/tests/sys/kqueue/libkqueue/timer.c Modified: head/lib/libc/sys/kqueue.2 ============================================================================== --- head/lib/libc/sys/kqueue.2 Fri Jul 27 13:11:05 2018 (r336760) +++ head/lib/libc/sys/kqueue.2 Fri Jul 27 13:49:17 2018 (r336761) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd June 22, 2017 +.Dd July 27, 2018 .Dt KQUEUE 2 .Os .Sh NAME @@ -154,7 +154,7 @@ struct kevent { u_int fflags; /* filter flag value */ int64_t data; /* filter data value */ void *udata; /* opaque user data identifier */ - uint64_t ext[4]; /* extentions */ + uint64_t ext[4]; /* extensions */ }; .Ed .Pp @@ -355,8 +355,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 . @@ -501,7 +501,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 @@ -558,6 +558,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 @@ -604,9 +611,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, @@ -782,7 +789,7 @@ The value is limited to 24 hours; longer timeouts will be silently reinterpreted as 24 hours. .Pp -In versions older than +In versions older than .Fx 12.0 , .In sys/event.h failed to parse without including Modified: head/sys/kern/kern_event.c ============================================================================== --- head/sys/kern/kern_event.c Fri Jul 27 13:11:05 2018 (r336760) +++ head/sys/kern/kern_event.c Fri Jul 27 13:49:17 2018 (r336761) @@ -162,6 +162,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); @@ -190,6 +194,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, @@ -699,30 +704,45 @@ 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; struct bintime bt; - sbintime_t to, sbt; - unsigned int ncallouts; + sbintime_t sbt; 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 | NOTE_ABSTIME)) != 0) return (EINVAL); - to = timer2sbintime(kn->kn_sdata, kn->kn_sfflags); + *to = timer2sbintime(kn->kn_sdata, kn->kn_sfflags); if ((kn->kn_sfflags & NOTE_ABSTIME) != 0) { getboottimebin(&bt); sbt = bttosbt(bt); - to -= sbt; + *to -= sbt; } - if (to < 0) + 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) @@ -734,6 +754,17 @@ 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; if ((kn->kn_sfflags & NOTE_ABSTIME) != 0) { kc->next = to; kc->to = 0; @@ -743,8 +774,6 @@ filt_timerattach(struct knote *kn) } callout_reset_sbt_on(&kc->c, kc->next, 0, filt_timerexpire, kn, PCPU_GET(cpuid), C_ABSOLUTE); - - return (0); } static void @@ -759,6 +788,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: head/tests/sys/kqueue/libkqueue/common.h ============================================================================== --- head/tests/sys/kqueue/libkqueue/common.h Fri Jul 27 13:11:05 2018 (r336760) +++ head/tests/sys/kqueue/libkqueue/common.h Fri Jul 27 13:49:17 2018 (r336761) @@ -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: head/tests/sys/kqueue/libkqueue/main.c ============================================================================== --- head/tests/sys/kqueue/libkqueue/main.c Fri Jul 27 13:11:05 2018 (r336760) +++ head/tests/sys/kqueue/libkqueue/main.c Fri Jul 27 13:49:17 2018 (r336761) @@ -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: head/tests/sys/kqueue/libkqueue/timer.c ============================================================================== --- head/tests/sys/kqueue/libkqueue/timer.c Fri Jul 27 13:11:05 2018 (r336760) +++ head/tests/sys/kqueue/libkqueue/timer.c Fri Jul 27 13:49:17 2018 (r336761) @@ -19,9 +19,59 @@ #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)"; @@ -189,7 +239,7 @@ test_abstime(void) kev.fflags = 0; kevent_cmp(&kev, kevent_get(kqfd)); if (time(NULL) < when + timeout) - err(1, "too early %jd %jd", time(), when + timeout); + err(1, "too early %jd %jd", time(NULL), when + timeout); /* Check if the event occurs again */ sleep(3); @@ -198,16 +248,283 @@ test_abstime(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(); - test_abstime(); - disable_and_enable(); + test_kevent_timer_add(); + test_kevent_timer_del(); + test_kevent_timer_get(); + test_oneshot(); + test_periodic(); + test_abstime(); + 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?201807271349.w6RDnHoX070437>