kqueue: assert that kqueue knote lists own the knotes

[kevans]

sys/kern/kern_event.c

3149

It just occurred to me here that the kqueue_fork bits introduced a divergence that some other parts don't expect: before this, KN_MARKERs could only ever appear on kq->kq_head. I think this only affects knote_fdclose, but there's no damage in practice because you always mark the preceding entry as influx (so we'll always go into fluxwait if the marker's next, and the marker will likely have moved before we drop it out from underneath the fork copy).

3162

Where did this path acquire a ref? I only see a kqueue_acquire() in the kqueue_fork_alloc(), but we did probably want one before we started copying to play nicely with a concurrent kqueue_drain

[kib]

sys/kern/kern_event.c

3149

Do you mean that we should skip markers more systematically?

3162

The filedescriptor copy for DFLAG_FORK files occurs in two stages. First, the call to the fo_fork method with non-NULL src file * pointer is done. This is where the reference on the source kq is obtained. It cannot be closed since then. Also, the destination kq is allocated.

Next, after all target kq's are allocated, we do the second pass over the filedesc table and call fo_fork() with NULL src file *. There, the actual copying of the knotes happen. This two-stage copying is required because knotes are filedesc-based, not file. So we need to have all filedesriptors allocated in the destination filedesc table, including the copied kqueues, since kqueue might have a knote that watches another kqueue.

So I believe that the source kqueue is referenced enough there.

[kevans]

sys/kern/kern_event.c

3149

I think we probably should, yes, or at least assert in knote_fdclose() that we're not looking at a marker to be sure.

3162

Ah, sorry- that makes sense, thanks.

[kib]

sys/kern/kern_event.c

3149

After looking at this some more, I do not think that the iteration in knote_fdclose() can ever see a marker, regardless of the flux state. The fdcopy() fo_fork() calls happen under the filedesc shared-locked, while knote_fdclose() is guaranteed to be under the exclusive filedesc lock.

Still, I think it is prudent to explicitly skip markers in all loops over kn_link, to have less surprises. And kern_proc_kqueue_report() seems to have the bug with markers, they must be skipped there.

I will write the patch after you commit the knote_fdclose() changes, to avoid the conflict.

[markj]

A comment on the commit message: using a generic subsystem name in the title, like "kqueue", is clearer IMO than "kern/kern_event.c". It's shorter and easier to understand at a glance, since most people know what kqueue is but many people probably don't know offhand that it's implemented in a file called kern_event.c.

sys/kern/kern_event.c

2944

When is it possible to have SLIST_EMPTY(list) here?

[kib]

sys/kern/kern_event.c

2944

I did already looked at this check, and I do agree that it should never happen.

I also considered changing SLIST_REMOVE() to explicit list walk, because right now if the knote is not found on the slist, nothing happens. At least this should be asserted.

[markj]

sys/kern/kern_event.c

2944

Yes, an assertion would be good. Actually I am surprised that SLIST_REMOVE does not assert that it found the element. IMO it should, though some users may rely on that behaviour.

[kib]

sys/kern/kern_event.c

2944

In fact I was wrong. If SLIST_REMOVE() did not found the element, it should deref NULL and and panic.

And I highly suspect now that the SLIST_EMPTY() check was a workaround for the issue we are trying to track down in the PR 293382. It comes from 69cd28dacb8e71cbd62f1f4, but the commit message does not mention the reason for the added check.

Also, I already removed the check in the version of the debug patch I sent to the person who started the PR. Will see where it goes.

[kib]

sys/kern/kern_event.c

3149

And filt_liodetach() has the similar patter for knlist (instead of klist).