Differential D26709
domain: explicitly track domains with fasttimo/slowtimo callbacks Authored by kevans on Oct 7 2020, 6:17 PM. Tags None Referenced Files
Details
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.
Diff Detail
Event TimelineComment Actions 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. Comment Actions 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).
Comment Actions This looks fine to my cursory reading. Again, someone with stake in the area will have to ACK it. Comment Actions 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. Comment Actions Ok, yeah, that makes sense- thanks! I'll pull the two CK_LIST_REMOVES back out into pf_proto_unregister.
Comment Actions 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.
Comment Actions 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.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||