domain: explicitly track domains with fasttimo/slowtimo callbacks

[kevans]

Summary

This adds two new lists, one for domains with fasttimo and one for domains with slowtimo. This eliminates the immediate branch/atomics in pffasttimo and pfslowtimo as well as reduces the number of domains/protocols that need to be iterated over each time.

I may squash this with D25459 prior to commit.

[mjg]

Why not a step further and a global list of protocols? Regardless, this will need a buy in from someone from the network stack. I don't have stake here.

fwiw, cursory reading suggests the patch is fine albeit the CK stuff is right now a little spurious.

Bonus points if this is not even a list but a big enough(tm) array. Again, rather optional at this stage.

[kevans]

Slight improvement; move our new lists from struct domain to struct protosw (still not really what mjg suggested). This simplifies a lot of it as we're now really only tracking the specific protosw that need action, and it removes all branching directly from pg{slow,fast}timo as we take advantage of the net epoch to remove unloaded protocols.

Tested only lightly with kldload pfsync; kldunload pfsync; which triggers the new bits (but doesn't have any fasttimo/slowtimo callbacks).

[kevans]

sys/kern/uipc_domain.c

452

I will likely commit the conversion of this assignment into the KASSERT that pr_domain == dp ahead of time.

[mjg]

This looks fine to my cursory reading. Again, someone with stake in the area will have to ACK it.

[mjg]

Well I'm note sure about NET_EPOCH_CALL semantics.

The general idea is that you remove protos from the list while having the relevant lock held and then wait for epoch to drain so that you know anybody who could possibly see them has left.

Does not look like is held over unregister in the current code?

CK_LIST_REMOVE in pf_proto_unregister while under dom_mtx will probably do the trick.

[kevans]

Ok, yeah, that makes sense- thanks! I'll pull the two CK_LIST_REMOVES back out into pf_proto_unregister.

[kevans]

Remove from lists prior to the callback

[markj]

sys/kern/uipc_domain.c

430

Doesn't this need to happen under dom_mtx?

[kevans]

sys/kern/uipc_domain.c

430

Yeah, I suspect this was overlooked when I removed the removal from the epoch callback below.

[kevans]

Push additions to the fasttimo/slowtimo list at register time back under the dom_mtx. I note that the fasttimo/slowtimo callouts can race here and invoke the callbacks before we initialize the newly-registered protocol (protosw_init), but this is not new -- the in-tree variant has the same problem, just on a probably-harder-to-hit scale since it iterates over all protocols in all domains.

There's also a window where we can be invoking callbacks and a dynamic protocol is deregistered in the process. I haven't completely thought about how this plays out, but my initial impression is that this is probably relatively harmless. If it gets removed as we're invoking the callback, that's fine -- we don't complete de-orbit of the protocol until the epoch has elapsed, presumably the callouts in the worst case here may terminate a little early during one invocation as the next member has been cleared so we can't observe the rest of the list.

[kevans]

sys/kern/uipc_domain.c

441

This isn't under the dom_mtx either, so the protocol should probably be getting reset later so we don't discover it and end up with a problem in a concurrent pf_proto_register against the same domain.

[kevans]

Defer the setting of pr->pr_protocol at deorbit time until the end, and throw an atomic_thread_fence_rel() before it. It seems generally OK to do this locklessly, as long as the other stores have all completed before pf_proto_register() observes the entry as a PR_SPACER as it will immediately bcopy() into the spacer.

pf_proto_register could fail if there's a deregistration in process that would otherwise be freeing up the spacer, but hasn't yet because it's waiting on the epoch. This seems OK.

[mjg]

sys/kern/uipc_domain.c

459

So what prevents taking the lock? This looks really weird as is.

[kevans]

sys/kern/uipc_domain.c

459

Can't take a lock in an epoch callback, iirc?

[kevans]

sys/kern/uipc_domain.c

459

(and we need epoch to ensure stability in the detaching domain somewhere along the way)

[mjg]

sys/kern/uipc_domain.c

459

I don't know if you can, it should be easy to check. Worst case you can use it to postpone the work to a taskqueue which can do it.

[kevans]

sys/kern/uipc_domain.c

459

Deferring to a taskqueue in an already deferred epoch callback for the sole purpose of taking the mutex feels even more convoluted. We only care for precisely the one place this comment mentions.