Thanks. The original reporter of the bug discovered that too.

This one boots, but panics when I kldload pfsync:

I have no objections to the patch, but I don't know enough about HFSC to meaningfully review this, I'm afraid.

I'm pretty sure that ipv6_activate_all_interfaces="YES" in /etc/rc.conf is the trigger for the previous panic.
That also matches nicely with the backtrace. Presumably nd6_dad_timer() doesn't enter the NET_EPOCH.

This panics my test vm:

LGTM

Did you happen to do any benchmarking on this? I'd have expected "Sync pfil hooks epoch(9)" to give us a (small) performance improvement, but my initial test shows a small reduction in forwarding performance (with pf loaded).

This panics during the netipsec and pf regression tests:

To be clear: I don't object to your change. It's just that ifconfig is a bit of a mess and any opportunity for cleanup is tempting. Especially if I don't have to do it.

Yeah, looking at my original commit I did manage to lose that flag. I’ve not tested yet, but this patch is almost certainly correct.

Also, I’d really, really like some basic tests for altq. It should be reasonably straightforward to do something based on the existing pf tests. At least, if altq can work on top of epairs.

LGTM.

I’ll try to review asap, but I’m on holiday wirh limited internet access right now.

In D18373#392613, @eri wrote:

Not a blocker but:

• It would also be nice to have measure if the other side can keep up with the blast of state updates now?
• Even better, provide the same bucket mechanism on reception so it can be distributed on the various cores

In D18373#392610, @eri wrote:

Can you please measure the latency of syncing states with this change against previous latency?

Fix build with ! VIMAGE

Address man page remark.

Minor improvements, style and such.
Small man page addition.

Abandoned in favour of D18373.

Abandoned in favour of D18373.

Abandoned in favour of D18373.

Single pfsyncintr swi, rather than one per bucket.

In D18373#390595, @mjg wrote:

once more i don't have a full picture so can't give a proper review.

Well, the remarks you've had have been pretty helpful so far.

And here's my flame graph: https://people.freebsd.org/~kp/pfsync/buckets.svg

This is a slightly rough first version. I'm sure there are some remaining style issues (and there might be cleanup issues too).

I have an alternative version in https://reviews.freebsd.org/D18373.
It splits pfsync up into buckets, with their own lock. Performance is slightly better than this, and it's in many ways a simpler change.

In D17994#389260, @eri wrote:

Can you add some text to the manual pages for documenting the feature? Possibly linking to some example?

In D17992#387985, @mjg wrote:

Single-threaded processing is definitely a bottle neck. Perhaps you can have more than one thread pushing stuff and/or some of it can be done by submitting threads instead.

In D17992#386171, @mjg wrote:

So both ring and swi kicking code are significant players. I think a simple and probably good enough solution would just add more rings, perhaps based on the number of hardware threads. Assuming the traffic is hashed to distribute among them, the rings could mostly remain unshared with unrelated threads. Sending out of the traffic would just combine data from all rings.

In D17992#385087, @eri wrote:

What needs to be considered here is the assumption of pfsync is that a state created in pf will be synched at shortest possible cycle to the cluster member.
By defering that assumption is relaxed so figuring out baselines of before and after this change would make this more easy to reason about.

Here are flame graphs for my test setup:

• plain forwarding
• pf only
• unpatched pfsync
• pfsync with this (and the next) patch

I've been thinking about what we could do to split up the lock, rather than take this approach, and I'm not sure how.

For what it's worth, OpenBSD don't drop here but henning did add a /* XXX why clear instead of drop? */ comment a few years ago.

In D17992#384518, @mjg wrote:

I think the approach taken here is iffy. Basic problem with this is that even if there is no lock contention anymore, you are still suffering from bouncing cache lines. Also swi_sched probably does not appreciate being called very often.

My performance tests show an improvement of 80-90% with this change. The swi_sched is costly, yes, but it's a pretty significant improvement in throughput.

Formatting change.

In D17505#379676, @glebius wrote:

I'm not absolutely sure that all possible races are fixed. There still could be dangling ifnet pointers. But that's up to your justification. If you are sure everything is covered, feel free to remove.

I'm not currently aware of any remaining issues here. It's certainly possible that some still exist, but I've seen no reports and the tests don't provoke them.
I think of this change mostly as a 'I will support this now.' statement.

Committed as r339836

Assorted pf VIMAGE fixes have been done, and pf is now usable inside VIMAGE jails.

An alternative approach (VIMAGE based) was committed instead.

Make :0 work without ()

We also don't want to not get notified and have an interface hanging around for ever given pf never releases a reference?

Yes. Especially because pfsync currently does a lot of work (including taking a single lock, which really kills throughput) even if it won't actually end up sending the sync packets out.
It should be possible to change pfsync so we don't virtualise the callback pointers and instead immediately check if it's enabled and return if not. We would have to be careful about locking around the up/down (and defer) flags, so it'd be a larger change than this.

Fix comment style

The following also resolves the use-after-free issue.

I’ll try to review in the next day or two, but it needs tests ;) (in a separte commit)
I think there are currently no functional tests for if_bridge, but it should be pretty straightforward to spin up a vnet jail, create a bridge in it and then use libifconfig to add/remove interfaces.
It’d have the very nice side-effect of getting us some basic bridge tests too.

Each vnet may choose to set pfsync0 down, which (see pfsyncioctl() / SIOCSIFFLAGS) set or clear these callbacks.
And while it's okay for a single vnet to decide it doesn't want pfsync to be enabled, it's not okay for that decision to affect all vnets.

Test plan:
kldload pfsync
cd /usr/tests/sys/netpfil/pf
kyua test