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

Abandon at the request of the submitter.

Commandeering so this can be abandoned.

OpenBSD use M_FLOWID_VALID to indicate if the flowid is set. FreeBSD does it slightly differently, and uses a separate field, usually accessed via M_HASHTYPE_SET(), M_HASHTYPE_GET(), M_HASHTYPE_ISHASH(), ... macros.

It'd be really awesome if you could also see about writing some basic tests (both as pfctl parser tests, to check we can now set higher bitrates for shaping, and as a basic test that shaping actually happens), but that's certainly not blocking.

Two minor remarks, at first glance. I've not dug into it at great depth, but your approach looks good and I've not seen any big problems.

You're right that the ternary operator is now redundant. I wonder if it wouldn't be better to just get rid of ssidmax and use IEEE80211_NWID_LEN everywhere in its place:

Patches (almost) always go into CURRENT first, before they can be merged back into stable/11 and stable/10.

In D16613#353056, @nonesuch_longcount.org wrote:

Kristof , I am going to retest using the simplified patch. Do I need to resubmit it in this form ?

No, that's okay. I've got the patch and can commit it once we've established it fixes your problem (and doesn't cause any new ones).

This change is probably also required for pf_test6(). OpenBSD unified pf_test() and pf_test6(), but we haven't done that yet.

I don't think the comparison is quite correct, in that the '== 0' will make it do the reverse of what you what you intend.

I don't know enough about the build system to comment on the makefile changes, but it builds as expected, and I have no objection to moving the pf.os file.

Looks good. Thanks for this. Let's get it in before 12.

In D15922#339443, @kevans wrote:

I discussed this with Farhan in IRC for a bit- I like the idea of actually respecting SSIDEncoding (the SSID EXTCAP, from what I can tell, by the time it hits us). I know for that hostap sets it given proper configuration- do you have any insight as to how many consumer APs actually set it or expose it properly? I went to go plumb this out and save it to the vap, only to discover my AP doesn't actually offer any way to set it.

I'm not sure if I object to this change or not, but it's worth noting that SSIDs are not necessarily UTF-8 strings. Unless the SSIDEncoding is set it is 0-32 octets. Having 0 bytes in the middle of the SSID is valid (though I'd be very surprised if that actually worked on more than a handful of devices). If SSIDEncoding is set it is indeed a UTF-8 string.
For additional fun Microsoft got this wrong and several Windows versions interpret the SSID as being Latin1 encoded.

I'd prefer the sysctl to reject values that are out of range.
What do you think about this: https://people.freebsd.org/~kp/patches/D14536.patch ?

Looks good at first glance.
I like the move of the sys/rmlock.h include into pfvar.h, so the header becomes more self-contained (i.e. you don't need extra includes to make your include of pfvar.h work). I'm running test builds for that, because I thought it broke when I tried. Perhaps I messed something else up though.

In D14536#316763, @darkfiberiru_gmail.com wrote:

Sorry I thought I had already replied to this. I have issues with keeping that as the default value as it has been deprecated since 1998. As such it's not really a sane default and not compatible with much modern routing gear. The setting I choose comes directly our of the RFC for this type of traffic.

Well, as I said, I have no views on what the value should be. I defaulted to keeping the old value, but if you've got a good reason to change it that's fine by me.