Copy kqueues into the child on fork

[kib]

Summary

Add KQUEUE_CPONFORK flag for kqueue1(2), that marks the created kqueue as requiring copy on fork into the child' file descriptor table.

This means that a new kqueue is created at the same fd index as the parent' kqueue, and all registered events are copied into the new kqueue (when possible). The current active events list is also duplicated.

Another sore place is the removal on the marker from the knlist, which is very high-cost for SLIST (the second XXXKIB place).

[kib]

Missed check for KQ_CPONFORK

[kib]

Add f_copy; properly copy timers.
Many fixes for generic knote copy.

[markj]

sys/kern/kern_event.c

3037

Shouldn't we ignore knotes corresponding to fds with O_CLOFORK set?

[kib]

sys/kern/kern_event.c

3037

I thought about translating KQUEUE_CPONFORK into ~O_CLOFORK and vice versa, but there are some details. I might get to this soon.

[kib]

sys/kern/kern_event.c

3037

Oh, you mean knotes. Right, I think this is good idea.

[markj]

sys/kern/kern_event.c

3037

I mean, suppose the parent process has a EVFILT_READ knote for fd 3, and fd 3 is flagged O_CLOFORK, so it will be closed in the child process. Then, this knote must not be inherited, I think.

[kib]

Ignore fd-based knotes for which fd was not copied.
Tweak locking to avoid witness warning about dup filedesc lock.

[kib]

Handle copy for vfs/vnode filters

[kib]

Skip copying of influx notes (that are infux not due to scan).
Add copy method for netmap notes.

[kib]

Call kqueue_expand() as needed. Not doing so has profound consequences, including not calling knote_drop() on close of forked kqueue.

Allocate forked kqueue fd with the ref count on 1 instead of 2. Using '2' hide the issue with missed knote_drop().

Harmonize internal interfaces for copying specific knote.

[kib]

From my current PoV, this should be more or less finished implementation. In particular, I reviewed all filters for the need of special copy handling.

[kib]

In D52045#1193293, @kib wrote:

From my current PoV, this should be more or less finished implementation. In particular, I reviewed all filters for the need of special copy handling.

Ping?

[markj]

In D52045#1199301, @kib wrote:

In D52045#1193293, @kib wrote:

From my current PoV, this should be more or less finished implementation. In particular, I reviewed all filters for the need of special copy handling.

Ping?

Sorry, I will try to get to this patch later this week.

[kib]

Rebase. Document KQUEUE_CPONFORK

[arrowd]

sys/compat/linux/linux_event.c

107

Shouldn't the third arg be true in this call? In Linux epoll fds are copied across the forks.

[kib]

sys/compat/linux/linux_event.c

107

Might be. For now I intend to keep existing behavior.

[kib]

Bug fixes after Peter' testing.

Keep source kq pointer in the copy kq until copying phase two is done.

[kib]

I intend to commit this early next week.

[markj]

lib/libsys/kqueue.2

105

109

111

Maybe, "The kqueue descriptor will be inherited by the child, i.e., the numeric value of the descriptor will remain the same."?

115

sys/kern/kern_event.c

3013

Strange indentation here, is it from debugging?

3124

This looks like some debugging.

[kib]

sys/kern/kern_event.c

3013

Indeed this is debugging, I forgot to drop the patch when generating the diff.

[kib]

Man page editing

[markj]

lib/libsys/kqueue.2

109

Maybe mention that this is contrary to how all other descriptors are treated upon fork(), i.e., we make shallow copies instead.

sys/kern/kern_descrip.c

2539–2554

This comment should be removed or enhanced to explain what is actually happening.

2582

If you handle the !fork_pass case here instead, you can reduce indentation below.

2584

Maybe make a local variable for ofde->fde_file->f_ops so that the lines aren't so long.

2596

Why do we handle DFLAG_FORK in a separate pass? Why not call fo_fork in the first loop?

sys/kern/kern_event.c

966

Inheriting the cpuid like this doesn't really make sense. Ideally the callout will fire on the same CPU where it was scheduled from. Maybe filt_timerstart() should also reset kc->cpuid = PCPU_GET(cpuid).

3089

It is not too unusual for a queue to have several thousand knotes. I suspect this will be slow indeed.

[kib]

sys/kern/kern_descrip.c

2582

Each FILEDESC_FOREACH_FDE() needs its own block scope unfortunately.

2596

This is the commit message for DFLAG_FORK change:

struct file: add DFLAG_FORK, indicate copying on fork

instead of referencing or ignoring.

The pass to handle copying is performed after all passable files are
installed into the new file descriptor's table, since one of the future
consumers of this interface (kqueue) needs all passable file descriptors
already valid. For the same reason the copying is done in two passes
by itself, first allowing copyable files to be copied, and then giving
the subsystems a second chance to really fill the copied file backing
structure with the content.

[MFC note: use fo_spares for fo_fork]

(and note that kqueue might need the fd number to reference other kqueue)

sys/kern/kern_event.c

3089

It is worse at more significant place, knlist_remove_kq().

[kib]

Changes due to the latest comments.

[kib]

Wrap the first stage of the fork pass into the original pass over source fd table.

[kib]

Some editing to the comment.

[markj]

The code changes seem ok to me. This should really have some regression tests. Even some simple ones which exercise the f_copy routine for various filters, and verify that knotes for O_CLOFORK fds are handled properly, would be much better than nothing.

lib/libsys/kqueue.2

120

You might mention somewhere that knotes corresponding to O_CLOFORK fds will not be inherited.

sys/kern/kern_descrip.c

2588

sys/kern/kern_event.c

3089

knlist_remove_kq() is just removing a single knote though.

[kib]

In D52045#1213312, @markj wrote:

This should really have some regression tests. Even some simple ones which exercise the f_copy routine for various filters, and verify that knotes for O_CLOFORK fds are handled properly, would be much better than nothing.

There is a program I posted which I use for testing the change. I believe Peter took it into stress2 already, staged.

I will convert the program to atf someday, but I do not want this to delay the commit. I promise the conversion.

sys/kern/kern_event.c

3089

But all knotes are removed eventually, so the same amount of work is spread over the time.

[kib]

Man page update: explain that knotes for closed fds are not copied.

[kib]

Merge two if's.

[markj]

lib/libsys/kqueue.2

105

sys/kern/kern_event.c

3041

If someone adds a filter which requires some specialized code in order to copy a knote, and forgets to implement f_copy, there will be some hard-to-debug memory corruption most likely. That is, this is a fail-open design.

I wonder if we can do better? Maybe skip the knote if f_copy is note defined, and have a no-op f_copy implementation that filters can use.

3089

It depends on the order of the removals. Removing the first element of the list until the list is empty is an O(n) operation. Removing the last element of the list until the list is empty is an O(n^2) operation. Traversal here is an O(n^2) operation always. I suspect it will eventually cause pain for someone.