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.

In D15018#316088, @cem wrote:

Looks good in general. It seems that some cases use WOULD_OVERFLOW as a secondary check and some don't. Is there any reason for that?

Good catch, I missed it in the first two cases because that used to be a fixed 65k value. Now that it's user configurable it needs the check too.

Fix static / extern mismatch

Add missing WOULD_OVERFLOW checks.

Committed as r332101

Committed as r332102

What problem does this fix?

Keep the old hooks so other pfil users don't need to change. Allow pf to use the new style of hook, with the flags argument.

Looks sane to me.

I have no specific views on what the value should be. The remark mostly comes from the desire to avoid having the kernel enforce policy. It's only a small extra step here for more flexibility.
I think I'd make net.inet.carp.dscp be an integer, with a default value of (the value of) IPTOS_LOWDELAY.

Would it make sense to set the DSCP value to the value configured in 'net.inet.carp.dscp' rather than a hardcoded value?

In D11137#297170, @olivier wrote:

Any news about pushing this change to head ?

In D13715#296796, @ae wrote:

I think someone from pfSense or openSense already reworked PF to work "inplace" like ipfw does, i.e. it returns mbuf back to the function from where pfil was invoked.

In D13715#296702, @eri wrote:

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

pf(4) already has a loop detection mechanism that i created to support divert(9) and dummunet(9), not sure if the later ever made it into FreeBSD.

Now I'm confused. This isn't about loop detection. This is about detecting if a PFIL_OUT packet is being forwarded or output.

Good catch.

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

Based around a suggestion from Kyle Evans (who also did all of the work), introduce a flags variable to the pfil callbacks. Keep using PFIL_OUT for forwarded packets, but set the PFIL_FWD flag for them. This allows pf to work out if a packet is being forwarded or not, with essentially no changes to other netpfil consumers.

In D13766#290617, @erj wrote:

Is this going to be MFC'd into stable/10 and/or stable/11?

ae@ is probably the best person to talk to about ipfw, so you may want to cc him too.

In D13715#288702, @eri wrote:

While this is needed i do not agree that the modifications on the stack and packet filters should be so hackish.

More context. No changes to the diff.

Use the OpenBSD mallocarray implementation.

Add __alloc_size attributes.

Update malloc.9 man page.

I have to admit I initially wanted to call it calloc(), but it turns out ZFS already has calloc(size_t, size_t) in sys/cddl/compat/opensolaris/sys/kmem.h, so that failed to build.

Removed incorrect __alloc_size attribute.

I'll try again once I see a commit go by that looks like it'd fix that. Do feel free to remind me if I manage to miss it (or forget).

It looks like something's still wrong:

Well, I suppose this is good, in that it shows why these tests are useful:

Oh, also: if some of the tests are known to fail we should mark them as such until the issue is fixed.
See 'atf_expect_fail'

How to add the case of with and without AESNI without rewriting all these tests?

I suspect this will also address PR 222647.

This is similar to how I fixed this problem for pf.

Similar results for v6:

With a larger file (102400 bytes) the difference indeed shrinks:

I found some crusty old hardware to run a test on. This is nginx serving its default index page (612 bytes of data).
Test client is wrk. I played around with the number of connections and threads briefly, but didn't see a major difference (in the non-vimage performance, I've not compared vimage there).

In D12639#262598, @bz wrote:

@jtl I am not aware of anyone done a end-host, e.g., webserver kind of performance measurement. No one wanted to volunteer so far.

Thumbs up. Let's do this.

Sorry, I missed that remark.

This should address all of the review remarks.

This builds on r323390, right?

In D11401#254776, @paggas1_yandex.com wrote:

I've made a change which I'm testing currently, running pkg with the -r
flag instead of chrooting. Will commit if it works.

Yes, that's even better.

While looking at this I also noticed that this file is (mostly) intended with spaces. FreeBSD style is to use tabs.

In D12011#248956, @bcr wrote:

Adding Kristof for confirming that this is the actual address in the code.

In D3272#247495, @adrian wrote:

yes please!

I've done a bit more testing, and these debug traces tell the story:

Hmm, good question. I thought I understood the failure flow fully, but now I'm not so sure.

In D11782#244283, @bz wrote:

Is this a consequence of https://svnweb.freebsd.org/base?view=revision&revision=297816 or independent of it?