Page MenuHomeFreeBSD
Differential D15778

Allow a EVFILT_TIMER kevent to be updated
ClosedPublic
Actions

Authored by dab on Jun 12 2018, 2:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 10:29 PM
Unknown Object (File)
Sun, Nov 17, 2:24 PM
Unknown Object (File)
Oct 8 2024, 6:11 AM
Unknown Object (File)
Oct 8 2024, 6:10 AM
Unknown Object (File)
Oct 8 2024, 6:10 AM
Unknown Object (File)
Oct 8 2024, 6:09 AM
Unknown Object (File)
Oct 8 2024, 6:09 AM
Unknown Object (File)
Oct 8 2024, 6:08 AM
View All 70 Files
Subscribers
badger
cem
imp
kib
vangyzen

Details

Reviewers
jmg
kib
Group Reviewers
manpages
Commits
rS336761: Allow a EVFILT_TIMER kevent to be updated.
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17503
Build 17330: arc lint + arc unit

Event Timeline

dab created this revision.Jun 12 2018, 2:19 PM
Herald added a reviewer: jmg. · View Herald TranscriptJun 12 2018, 2:19 PM
Herald added subscribers: badger, imp. · View Herald Transcript
Harbormaster completed remote builds in B17202: Diff 43656.Jun 12 2018, 2:19 PM
dab updated this revision to Diff 43763.Jun 14 2018, 3:34 PM

Fix a small panic-inducing typo.

Harbormaster completed remote builds in B17267: Diff 43763.Jun 14 2018, 3:34 PM
dab updated this revision to Diff 43764.Jun 14 2018, 3:38 PM

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

Harbormaster completed remote builds in B17269: Diff 43764.Jun 14 2018, 3:38 PM
dab edited the summary of this revision. (Show Details)Jun 14 2018, 5:40 PM
dab added a subscriber: vangyzen.Jun 20 2018, 9:56 PM
dab updated this revision to Diff 44193.Jun 20 2018, 11:02 PM

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.
Herald added a reviewer: manpages. · View Herald TranscriptJun 20 2018, 11:02 PM
Harbormaster completed remote builds in B17503: Diff 44193.Jun 20 2018, 11:02 PM
dab edited the summary of this revision. (Show Details)Jun 20 2018, 11:14 PM
dab updated this revision to Diff 45207.Jul 12 2018, 12:56 PM

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

Harbormaster completed remote builds in B17997: Diff 45207.Jul 12 2018, 12:56 PM
dab added a comment.Jul 17 2018, 3:50 PM

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 added a subscriber: kib.Jul 17 2018, 4:37 PM

@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 added inline comments.Jul 17 2018, 5:13 PM
sys/kern/kern_event.c
716–718

Style:

/*
 * The only ...
807

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
74

This chunk is unrelated and can be committed separately.

dab added inline comments.Jul 17 2018, 10:52 PM
sys/kern/kern_event.c
807

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 added a comment.Jul 18 2018, 12:39 PM

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

sys/kern/kern_event.c
807

I agree, influx should avoid the race.

dab mentioned this in rS336457: Make the definition of struct kevent in event.h match what the man page for….Jul 18 2018, 1:04 PM
dab marked an inline comment as done.Jul 18 2018, 1:07 PM
dab added inline comments.
sys/sys/event.h
74

Done in rS336457

dab marked 2 inline comments as done.Jul 18 2018, 1:07 PM
dab updated this revision to Diff 45456.Jul 18 2018, 1:09 PM

Fix style issue pointed out by @kib.

Harbormaster completed remote builds in B18117: Diff 45456.Jul 18 2018, 1:09 PM
dab marked 4 inline comments as done.Jul 18 2018, 1:10 PM
kib accepted this revision.Jul 18 2018, 3:10 PM
This revision is now accepted and ready to land.Jul 18 2018, 3:10 PM
Closed by commit rS336761: Allow a EVFILT_TIMER kevent to be updated. (authored by dab). · Explain WhyJul 27 2018, 1:49 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
23 lines
sys/
kern/
115 lines
sys/
8 lines
tests/
sys/
kqueue/
libkqueue/
4 lines
19 lines
333 lines

Diff 44193

View Options

lib/libc/sys/kqueue.2

Loading...
View Options

sys/kern/kern_event.c

Loading...
View Options

sys/sys/event.h

Loading...
View Options

tests/sys/kqueue/libkqueue/common.h

Loading...
View Options

tests/sys/kqueue/libkqueue/main.c

Loading...
View Options

tests/sys/kqueue/libkqueue/timer.c

Loading...