- User Since
- Sep 28 2014, 7:22 PM (219 w, 1 d)
Sat, Dec 8
Wed, Dec 5
Mon, Dec 3
Fix build with ! VIMAGE
Address man page remark.
Sun, Dec 2
Minor improvements, style and such.
Small man page addition.
Sat, Dec 1
Thu, Nov 29
Single pfsyncintr swi, rather than one per bucket.
Well, the remarks you've had have been pretty helpful so far.
Wed, Nov 28
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.
Mon, Nov 26
Thu, Nov 22
Wed, Nov 21
Sat, Nov 17
Here are flame graphs for my test setup:
- plain forwarding
- pf only
- unpatched pfsync
- pfsync with this (and the next) patch
Fri, Nov 16
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.
Thu, Nov 15
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.
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.
Wed, Nov 14
Nov 1 2018
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.
Oct 28 2018
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.
Oct 21 2018
Make :0 work without ()
Oct 20 2018
Oct 13 2018
We also don't want to not get notified and have an interface hanging around for ever given pf never releases a reference?
Oct 12 2018
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.
Oct 11 2018
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.
Oct 10 2018
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.
Sep 27 2018
Abandon at the request of the submitter.
Commandeering so this can be abandoned.
Sep 10 2018
Sep 1 2018
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.
Aug 28 2018
Aug 23 2018
Aug 18 2018
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.
Aug 11 2018
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:
Aug 7 2018
Patches (almost) always go into CURRENT first, before they can be merged back into stable/11 and stable/10.
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.
Aug 3 2018
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.
Jun 30 2018
Looks good. Thanks for this. Let's get it in before 12.
Jun 27 2018
Jun 20 2018
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.
Jun 9 2018
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 ?
May 20 2018
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.