Allow a EVFILT_TIMER kevent to be updated

[dab]

Summary

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 seems to violate 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 bug 214987, fixes the kqueue system to allow updating a timer
entry.

In his comments about the patch @cem noted that it "is probably
racy". I believe that I have now addressed the locking aspects
of this change.

PR: 214987

Test Plan

Run the test program attached to bug 214987. Re-run kqueue system tests.

[dab]

Fix a small panic-inducing typo.

[dab]

Add a test for the re-add of a timer.

[dab]

Update to do several things:

• When re-adding a timer, update regardless of whether the parameters differ or are the same as when originally added.

• 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 bring the FreeBSD and
macOS timer behaviors into agreement.

A few other things 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.

• Add some comments in sys/event.h that were documented in the kqueue(2) man page but missing from the actual .h file.

• Update the timer libkqueue system test to test for the updated timer behavior.

• Fix the 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).

• Define a few new helper functions in the libkqueue tests to assist in testing the new timer behavior.

[dab]

Updating to rebase off latest head revision (rS336219). No functional
change from previous diff.

[dab]

This review has been languishing for a bit.

Do any of the current reviewers (@jmg, manpages) or subscribers (@badger, @cem, @imp, @vangyzen) have feedback?

Is there someone else I should get to review this?

Thanks.

[cem]

@kib may have an opinion. I don't feel familiar enough with the kevent internals to comment on the code itself. The described semantics change seems fine to me.

[kib]

sys/kern/kern_event.c

716 ↗

(On Diff #45207)

Style:

/*
 * The only ...

807 ↗

(On Diff #45207)

I think that this is racy. You can have the callout running when you start draining, and then e.g. other thread could see the timer event activated.

I am not sure how much this can be declared to be the app race. But my gut feeling is that if we start draining the kqueue timers, we should use a lock to protect callout and make draining race-free.

sys/sys/event.h

91 ↗

(On Diff #45207)

This chunk is unrelated and can be committed separately.

[dab]

sys/kern/kern_event.c

807 ↗

(On Diff #45207)

I don't think so. If I understand the callout and kqueue code correctly (although there is a distinct possibility that I am missing something), the callout_drain() will wait for a currently executing callout (which would be filt_timerexpire()) to complete. While such an occurence would cause the knote to be activated by filt_timerexpire() and, hence, eligible to be returned to a waiting thread by kqueue_scan(), the knote is also at this point marked as "in_flux". Therefore, knote_scan() will delay the copyout() until the in_flux flag is deasserted. This will not happen until filt_timertouch() completes and by that time we will have thrown away the record of the timer expiration (the code immediately below this point).

Like I said, I may be missing something, so if I am please do point out my error.

[kib]

Fix the style issue and I think this is good to go in.

sys/kern/kern_event.c

807 ↗

(On Diff #45207)

I agree, influx should avoid the race.

[dab]

sys/sys/event.h

91 ↗

(On Diff #45207)

Done in rS336457

[dab]

Fix style issue pointed out by @kib.