Activate triggered kevents when they are enabled

[markj]

Summary

r274560 modified kevent_register() to avoid testing knotes for disabled
kevents, presumably as a micro-optimization. However, this check is done
before possibly enabling the disabled kevent with EV_ENABLE, which means
that the knote will not be activated by the kevent_register() call even
though the event is enabled when this call returns.

Note that a knote for disabled kevent may be activated independently of
a kevent_register() call; the problem occurs only when the event trigger
condition does not undergo a level transition while the event is
disabled.

This change also modifies an existing kqueue test to exercise this case.

Test Plan

kqueue tests pass.

This little program demonstrates the problem: https://people.freebsd.org/~markj/prs/206368/repro.c

Without the fix, the second kevent(2) call hangs.

[hiren]

Thanks for picking this up.

[kib]

Note that this works in very round-about way. The KN_DISABLED flag is still set while if (event) KNOTE_ACTIVATE(kn); is done. As result, the knote is not enqueued. But later, where the check for EV_ENABLE && KN_DISABLED is done, the active knote is put on queue and waiters are woken up. I am not sure how to restructure the code to make it less puzzling.

Also, should the case of EV_ENABLE and EV_DISABLE both set, checked somewhere ?

[markj]

In D5307#113191, @kib wrote:

Note that this works in very round-about way. The KN_DISABLED flag is still set while if (event) KNOTE_ACTIVATE(kn); is done. As result, the knote is not enqueued. But later, where the check for EV_ENABLE && KN_DISABLED is done, the active knote is put on queue and waiters are woken up. I am not sure how to restructure the code to make it less puzzling.

Do you see a problem with checking for EV_ENABLE before calling kn_fop->f_event()? That is, something like the following:

if ((kev->flags & EV_DISABLE) != 0)
    kn->kn_status |= KN_DISABLED;
else if ((kev->flags & EV_ENABLE) != 0)
    kn->kn_status &= ~KN_DISABLED;

if ((kn->kn_status & KN_DISABLED) == 0)
    event = kn->kn_op->f_event(kn, 0);
else
    event = 0;

KQ_LOCK(kq);
if (event)
    KNOTE_ACTIVATE(kn, 1);
else if ((kn->kn_status & (KN_ACTIVE | KN_QUEUED | KN_DISABLED)) == KN_ACTIVE)
    knote_enqueue(kn);
kn->status &= ~(KN_INFLUX | KN_SCAN);
KN_LIST_UNLOCK(kn);
KQ_UNLOCK_FLUX(kq);

In particular, we must take care to enqueue an active knote when transitioning from disabled to enabled. In this snippet we now do so before clearing KN_INFLUX and before releasing the knote list lock, but I don't see any problems with that.

Also, should the case of EV_ENABLE and EV_DISABLE both set, checked somewhere ?

I was wondering about this too. I note that the combination is currently harmless - it seems that specifying EV_ENABLE|EV_DISABLE is equivalent to just EV_ENABLE. Hypothetical future refactoring may introduce in a bug in this case though, so it should probably be handled explicitly. The beginning of kevent_register() seems like the right place for this check.

[kib]

In D5307#113329, @markj wrote:

Do you see a problem with checking for EV_ENABLE before calling kn_fop->f_event()? That is, something like the following:

I like this proposal.

[markj]

• Check for the combination of EV_ENABLE and EV_DISABLE
• Simplify queueing of active knotes in kqueue_register()

[kib]

sys/kern/kern_event.c

1125 ↗

(On Diff #13443)

Move this before initialization ? No need to zero out bunch of vars when not used due to error.

1338 ↗

(On Diff #13443)

If you replace KNOTE_ACTIVE() there with kn->kn_status |= KN_ACTIVE; and remove 'else' in the next line, then I think that the code would work the same way, with less indirections for reader to look up.

[markj]

sys/kern/kern_event.c

1338 ↗

(On Diff #13443)

Indeed, I find that a bit clearer.