Page MenuHomeFreeBSD

kristof (Kristof Provost)
User

Projects

User Details

User Since
Sep 28 2014, 7:22 PM (219 w, 1 d)

Recent Activity

Sat, Dec 8

kristof created D18483: pf: Fix endless loop on NAT exhaustion with sticky-address.
Sat, Dec 8, 3:07 PM

Wed, Dec 5

kristof added a comment to D18373: pfsync: Performance improvement.
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
Wed, Dec 5, 7:35 PM
kristof added a comment to D18373: pfsync: Performance improvement.
In D18373#392610, @eri wrote:

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

Wed, Dec 5, 7:25 PM

Mon, Dec 3

kristof updated the diff for D18373: pfsync: Performance improvement.

Fix build with ! VIMAGE

Mon, Dec 3, 11:04 AM
kristof updated the diff for D18373: pfsync: Performance improvement.

Address man page remark.

Mon, Dec 3, 8:38 AM

Sun, Dec 2

kristof updated the diff for D18373: pfsync: Performance improvement.

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

Sun, Dec 2, 5:20 PM
kristof abandoned D17994: pfsync: Insert static trace points.

Abandoned in favour of D18373.

Sun, Dec 2, 4:53 PM
kristof abandoned D17992: pfsync: Reduce contention on PFSYNC_LOCK().

Abandoned in favour of D18373.

Sun, Dec 2, 4:49 PM
kristof abandoned D17993: pfsync: Call pfsyncintr() directly from pfsync_msg_intr() rather than scheduling a swi.

Abandoned in favour of D18373.

Sun, Dec 2, 4:49 PM

Sat, Dec 1

kristof added inline comments to D17376: IPv6 Fragmentation Regression Tests from OpenBSD.
Sat, Dec 1, 5:24 PM

Thu, Nov 29

kristof updated the diff for D18373: pfsync: Performance improvement.

Single pfsyncintr swi, rather than one per bucket.

Thu, Nov 29, 9:16 PM
kristof added a comment to D18373: pfsync: Performance improvement.
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.

Thu, Nov 29, 7:08 PM

Wed, Nov 28

kristof added a comment to D18373: pfsync: Performance improvement.

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

Wed, Nov 28, 10:44 PM
kristof added a comment to D18373: pfsync: Performance improvement.

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

Wed, Nov 28, 10:40 PM
kristof added reviewers for D18373: pfsync: Performance improvement: network, mjg.
Wed, Nov 28, 10:40 PM
kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().

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.

Wed, Nov 28, 10:40 PM
kristof created D18373: pfsync: Performance improvement.
Wed, Nov 28, 10:37 PM

Mon, Nov 26

kristof added a comment to D17994: pfsync: Insert static trace points.
In D17994#389260, @eri wrote:

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

Mon, Nov 26, 10:28 AM

Thu, Nov 22

kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().
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.

Thu, Nov 22, 8:19 PM

Wed, Nov 21

kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().
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.

Wed, Nov 21, 7:22 PM

Sat, Nov 17

kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().
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.

Sat, Nov 17, 7:49 PM
kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().

Here are flame graphs for my test setup:

  • plain forwarding
  • pf only
  • unpatched pfsync
  • pfsync with this (and the next) patch
Sat, Nov 17, 1:51 PM

Fri, Nov 16

kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().

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.

Fri, Nov 16, 6:08 PM

Thu, Nov 15

kristof accepted D2266: I can find no reason to allow packets with both SYN and FIN bits set past this point in the code. The packet should be dropped and not massaged as it is here..

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.

Thu, Nov 15, 8:54 PM
kristof added a comment to D17992: pfsync: Reduce contention on PFSYNC_LOCK().
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.

Thu, Nov 15, 6:53 AM

Wed, Nov 14

kristof created D17994: pfsync: Insert static trace points.
Wed, Nov 14, 9:26 PM
kristof created D17993: pfsync: Call pfsyncintr() directly from pfsync_msg_intr() rather than scheduling a swi.
Wed, Nov 14, 9:25 PM
kristof created D17992: pfsync: Reduce contention on PFSYNC_LOCK().
Wed, Nov 14, 9:25 PM

Nov 1 2018

kristof updated the diff for D17734: pf: Limit the fragment entry queue length to 64 per bucket..

Formatting change.

Nov 1 2018, 10:58 AM
kristof added a comment to D17505: pfsync: Allow module to be unloaded.

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.

Nov 1 2018, 10:54 AM

Oct 28 2018

kristof added a reviewer for D17734: pf: Limit the fragment entry queue length to 64 per bucket.: network.
Oct 28 2018, 6:31 AM
kristof updated the summary of D17733: pf: Split the fragment reassembly queue into smaller parts.
Oct 28 2018, 6:31 AM
kristof updated the summary of D17732: pf: Count holes rather than fragments for reassembly.
Oct 28 2018, 6:30 AM
kristof created D17734: pf: Limit the fragment entry queue length to 64 per bucket..
Oct 28 2018, 6:30 AM
kristof created D17733: pf: Split the fragment reassembly queue into smaller parts.
Oct 28 2018, 6:30 AM
kristof created D17732: pf: Count holes rather than fragments for reassembly.
Oct 28 2018, 6:29 AM
kristof abandoned D17634: pf tests: Test ':0' ignoring link-local addresses.

Committed as r339836

Oct 28 2018, 6:00 AM
kristof closed D1309: VIMAGE PF fixes #1.

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

Oct 28 2018, 5:59 AM
kristof abandoned D11401: Kernel pf tests.

An alternative approach (VIMAGE based) was committed instead.

Oct 28 2018, 5:58 AM
kristof commandeered D11401: Kernel pf tests.
Oct 28 2018, 5:57 AM

Oct 21 2018

kristof updated the diff for D17633: pf: Make ':0' ignore link-local v6 addresses too.

Make :0 work without ()

Oct 21 2018, 7:46 PM
kristof added a reviewer for D17634: pf tests: Test ':0' ignoring link-local addresses: network.
Oct 21 2018, 12:38 AM
kristof added a reviewer for D17633: pf: Make ':0' ignore link-local v6 addresses too: network.
Oct 21 2018, 12:38 AM
kristof created D17634: pf tests: Test ':0' ignoring link-local addresses.
Oct 21 2018, 12:37 AM
kristof created D17633: pf: Make ':0' ignore link-local v6 addresses too.
Oct 21 2018, 12:37 AM

Oct 20 2018

kristof added a reviewer for D17625: tcpdump: Log uid on pflog interfaces: network.
Oct 20 2018, 6:32 PM
kristof created D17625: tcpdump: Log uid on pflog interfaces.
Oct 20 2018, 6:31 PM
kristof updated the summary of D17504: pf tests: Basic pfsync test.
Oct 20 2018, 6:21 PM
kristof added a reviewer for D17505: pfsync: Allow module to be unloaded: network.
Oct 20 2018, 6:20 PM

Oct 13 2018

kristof added a comment to D17500: Notify that the ifnet will go away, even on vnet shutdown.

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

Oct 13 2018, 5:10 PM
kristof added inline comments to D17522: libifconfig: Add initial support for if_bridge.
Oct 13 2018, 3:46 PM

Oct 12 2018

kristof added inline comments to D17522: libifconfig: Add initial support for if_bridge.
Oct 12 2018, 9:17 AM
kristof added a comment to D17499: pfsync: Make pfsync callbacks per-vnet.

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.

Oct 12 2018, 7:55 AM
kristof updated the diff for D17502: pfsync: Handle syncdev going away.

Fix comment style

Oct 12 2018, 7:52 AM
kristof added a comment to D17500: Notify that the ifnet will go away, even on vnet shutdown.

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

Oct 12 2018, 7:47 AM

Oct 11 2018

kristof added a comment to D17522: libifconfig: Add initial support for if_bridge.

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 11 2018, 5:52 PM

Oct 10 2018

kristof added a comment to D17499: pfsync: Make pfsync callbacks per-vnet.

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.

Oct 10 2018, 5:33 PM
kristof updated the summary of D17508: pfctl tests: Basic test case for PR 231323.
Oct 10 2018, 4:36 PM
kristof added a reviewer for D17507: pfctl: Dup strings: network.
Oct 10 2018, 4:36 PM
kristof created D17508: pfctl tests: Basic test case for PR 231323.
Oct 10 2018, 4:35 PM
kristof created D17507: pfctl: Dup strings.
Oct 10 2018, 4:35 PM
kristof added a comment to D17500: Notify that the ifnet will go away, even on vnet shutdown.

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

Oct 10 2018, 3:38 PM
kristof added a reviewer for D17506: pfsync: Add missing unlock: network.
Oct 10 2018, 3:21 PM
kristof created D17506: pfsync: Add missing unlock.
Oct 10 2018, 3:20 PM
kristof added a reviewer for D17502: pfsync: Handle syncdev going away: network.
Oct 10 2018, 3:20 PM
kristof added a reviewer for D17501: pfsync: Ensure uninit is done before pf: network.
Oct 10 2018, 3:20 PM
kristof added a reviewer for D17500: Notify that the ifnet will go away, even on vnet shutdown: network.
Oct 10 2018, 3:19 PM
kristof created D17505: pfsync: Allow module to be unloaded.
Oct 10 2018, 3:19 PM
kristof created D17504: pf tests: Basic pfsync test.
Oct 10 2018, 3:19 PM
kristof created D17502: pfsync: Handle syncdev going away.
Oct 10 2018, 3:18 PM
kristof created D17501: pfsync: Ensure uninit is done before pf.
Oct 10 2018, 3:18 PM
kristof created D17500: Notify that the ifnet will go away, even on vnet shutdown.
Oct 10 2018, 3:18 PM
kristof added a reviewer for D17499: pfsync: Make pfsync callbacks per-vnet: network.
Oct 10 2018, 3:17 PM
kristof created D17499: pfsync: Make pfsync callbacks per-vnet.
Oct 10 2018, 3:16 PM

Sep 27 2018

kristof abandoned D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

Abandon at the request of the submitter.

Sep 27 2018, 9:43 AM
kristof commandeered D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

Commandeering so this can be abandoned.

Sep 27 2018, 9:41 AM

Sep 10 2018

kristof accepted D17097: only lock row in pf purge thread when work to do.
Sep 10 2018, 8:51 AM

Sep 1 2018

kristof added a comment to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

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.

Sep 1 2018, 12:20 PM

Aug 28 2018

kristof accepted D16922: Add libxo(3) support to last(1)..
Aug 28 2018, 4:42 PM
kristof accepted D16919: Add libxo(3) support to lastlogin(8)..
Aug 28 2018, 1:15 PM
kristof accepted D16922: Add libxo(3) support to last(1)..
Aug 28 2018, 1:14 PM

Aug 23 2018

kristof accepted D16852: Update pfctl(8) tbrsize heuristic for high bandwidth interfaces.
Aug 23 2018, 9:16 AM

Aug 18 2018

kristof accepted D16782: Fix 32-bit queue bandwidth limitation in pf and in the ALTQ token bucket regulator and HFSC scheduler.
Aug 18 2018, 4:07 PM
kristof added a comment to D16782: Fix 32-bit queue bandwidth limitation in pf and in the ALTQ token bucket regulator and HFSC scheduler.

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.

Aug 18 2018, 2:59 PM
kristof added a comment to D16782: Fix 32-bit queue bandwidth limitation in pf and in the ALTQ token bucket regulator and HFSC scheduler.

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 18 2018, 2:58 PM

Aug 11 2018

kristof requested changes to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.
Aug 11 2018, 11:08 AM
kristof added inline comments to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.
Aug 11 2018, 11:08 AM
kristof accepted D16566: Drop the ternary operator for calculating ssid display length in list_scan().
Aug 11 2018, 10:09 AM
kristof added a comment to D16566: Drop the ternary operator for calculating ssid display length in list_scan().

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 11 2018, 8:46 AM

Aug 7 2018

kristof added a comment to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

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

Aug 7 2018, 1:24 PM
kristof added a comment to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

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).

Aug 7 2018, 12:28 PM
kristof added a comment to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

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

Aug 7 2018, 8:39 AM
kristof added a comment to D16613: Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD.

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 7 2018, 8:35 AM

Aug 3 2018

kristof added a comment to D16557: Move pf.os to sbin/pfctl/.

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.

Aug 3 2018, 8:05 PM

Jun 30 2018

kristof accepted D16076: pf: remove unused ioctls..

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

Jun 30 2018, 10:24 PM

Jun 27 2018

kristof added a comment to D15922: Added ssid values to render UTF-8 encoded characters in ifconfig(8).

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.

Jun 27 2018, 8:39 AM

Jun 20 2018

kristof added a comment to D15922: Added ssid values to render UTF-8 encoded characters in ifconfig(8).

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 20 2018, 8:17 AM

Jun 9 2018

kristof added a comment to D14536: Set DSCP bits in ip_carp.

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 ?

Jun 9 2018, 8:31 PM · network

May 20 2018

kristof added a comment to D15502: pf: Replace rwlock on PF_RULES_LOCK with rmlock.

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.

May 20 2018, 4:20 PM

Apr 11 2018

kristof added a comment to D14536: Set DSCP bits in ip_carp.

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.

Apr 11 2018, 3:22 PM · network