pf: optimize state key comparison on amd4

[mjg]

Summary

commit 59a03936d9870b9ded61221a6f123eeaff36dfde
Author: Mateusz Guzik <[e-mail]>
Date: Tue Oct 20 11:20:26 2026 +0000

pf: optimize state key comparison on amd4

commit 5de43eb78ac906dc1caedfde2977cee38dc0ab55
Author: Mateusz Guzik <[e-mail]>
Date: Tue Oct 20 11:19:50 2026 +0000

pf: CTASSERT on relevant offsets between pf_state_key and pf_state_key_cmp

[kp]

Do we have measurements of what difference this makes?

sys/net/pfvar.h

1032

This fails to build in userspace for me.

It probably just wants an #ifdef _KERNEL around it.

In file included from net_if_pfsync.c:1:
In file included from /usr/src/sys/net/if_pfsync.h:55:
/usr/src/sys/net/pfvar.h:1032:10: error: expected parameter declarator
 1032 | CTASSERT(offsetof(struct pf_state_key, addr) == offsetof(struct pf_state_key_cmp, addr));
      |          ^
/usr/src/sys/sys/_offsetof.h:16:32: note: expanded from macro 'offsetof'
   16 | #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
      |                                ^

[mjg]

• guard CTASSERT with _KERNEL. verified it fixes buildworld

[mjg]

I did not see any point in benchmarking this, it is strictly less work in the fast path.

"unofficial" numbers showed up internally anyway and they show a small improvement fwiw

[mjg]

sigh, use correct diff

[glebius]

sys/net/pfvar.h

1033–1036

CTASSERT() is a relic from pre-C11 times. _Static_assert() should be used in new code.

sys/netpfil/pf/pf.c

940–942

Why not just substitute bcmp to memcmp then? This also goes inline with overall trend in the kernel.

[glebius]

In D60894#1397813, @mjg wrote:

I did not see any point in benchmarking this, it is strictly less work in the fast path.

This is definitely not the right attitude when submitting patches that add more code and are declared as a performance improvement.

"unofficial" numbers showed up internally anyway and they show a small improvement fwiw

Unofficial numbers would be just fine, why hide them?

[mjg]

In D60894#1397856, @glebius wrote:

In D60894#1397813, @mjg wrote:

I did not see any point in benchmarking this, it is strictly less work in the fast path.

This is definitely not the right attitude when submitting patches that add more code and are declared as a performance improvement.

Given the nature of this specific patch, I disagree. In fact there is a host of possible changes adding code which don't warrant the effort needed for benchmarking (for example something which avoids a cache miss).

There is more involved work pending which will show up with bench numbers.

"unofficial" numbers showed up internally anyway and they show a small improvement fwiw

Unofficial numbers would be just fine, why hide them?

The person who did the measurement did one run, so they are not vouching for the accuracy. But here it is for interested: throughput while forwarding packets:

before: 10.89G, 3.696 MPPS
after: 11.07G, 3.748 MPPS

sys/net/pfvar.h

1033–1036

_Static_assert demands a message which de facto has to repeat the assert. Sounds like a step backwards to me for no gain.

sys/netpfil/pf/pf.c

940–942

I don't know what you mean.

If you mean s/pf_bcmp_state_key/pf_memcmp_state_key/, then that would be bogus because the new routine decidedly does not provide memcmp semantics -- it only returns an indicator whether buffers differ, not how.

If you mean to s/bcmp/memcmp/ in places comparing the key, that would be a nop because bcmp is already defined as __builtin_memcmp and the entire thing would compile to the same problem: a memcmp call.

Finally, the trend is bogus. Per the above, in case of a mismatch memcmp has fundamnetally more work to do as it needs to compute the relative difference, while there are consumers (like this one) which don't care about said detail. Interestingly clang even optimizes memcmp uses which merely check for a mismatch by treating them like bcmp and if need be emitting a call to that routine. This does not happen for kernel code because of the aforementioned #define bcmp __builtin_memcmp but maybe it should.

[mjg]

sys/netpfil/pf/pf.c

940–942

Perhaps it would help to clarify that the optimization "like memcmp on this platform" refers to how comparison is done -- subtract words and or differences between them.

This inline code wins in 2 ways:

• by eliding a function call
• by knowing at compilation time it is operating on a 40-byte buffer, avoiding runtime cost of figuring it out

[glebius]

sys/net/pfvar.h

1033–1036

While I don't agree with that, I actually can propose something different with all these pf keys. This double declarations bugged me since the original import from OpenBSD. There are two ways to solve that without providing two structures that should be carefully maintained to match (and thus require statis assertions):

1. Embed these foo_key into foo.
2. Remove foo_key. Add a #define foo_endcmp and use its offset in foo as comparison size. Just like we already have foo_endzero in different structs around the kernel. btw, D60948 adds one to pf, too.

This of course is out of scope of this review, just thoughts for pf people.

sys/netpfil/pf/pf.c

940–942

Sounds like all memcmp()s of size smaller than a cache line can be optimized this way? Are we going to sweep around entire kernel with changes like this one?

If this is indeed a serious win, we should consider making kernel memcmp() with a constant length a macro that would compile into inline in case of length is less or equal to cache line, and to function call instead.

[mjg]

sys/net/pfvar.h

1033–1036

I have no opinion.

sys/netpfil/pf/pf.c

940–942

I raised the issue of inline behavior with llvm people, see https://github.com/llvm/llvm-project/issues/233830 and someone pointed out the inline limit is configurable: https://godbolt.org/z/fn9doG8Kq

As you can see resulting asm operates on 16-byte chunks and ends up with 3 branches.

This is probably reasonable for a general purpose routine, because without explicitly checking for alignment between these buffers you don't know whey are you fetching an extra line. And you certainly don't know how often the comparison is expected to return a match.

In this particular case -- a hash -- vast majority of the time it should return a match on the first try, so the extra branches mostly hurt.

[kib]

sys/netpfil/pf/pf.c

948

As written, this is UB Perhaps it would be legal if you pass char * pointers and then cast to uint64_t in pf_bcmp_state_key().

But then, doing such rewrite bcmp->40+ LoC of code that is on the edge of UB and has the encouraging comment 'Interested parties ...', without even a benchmark, is IMO not productive.

1837

This might be UB, I am not sure about the letter of the standard there.

[glebius]

sys/netpfil/pf/pf.c

940–942

I raised the issue of inline behavior with llvm people, see https://github.com/llvm/llvm-project/issues/233830 and someone pointed out the inline limit is configurable: https://godbolt.org/z/fn9doG8Kq

I was actually about to suggest -mllvm -max-loads-per-memcmp=X a week ago, right before phabricator went down. You can add this to the pf.c CFLAGS in the Makefile, if the rest of pf(4) maintainers agree.

[kp]

sys/netpfil/pf/pf.c

940–942

That seems reasonable to me, assuming it won't break GCC builds. (And assuming we actually care about not breaking GCC builds?)

[kib]

sys/netpfil/pf/pf.c

940–942

Yes we do care about gcc.

In fact, another solution might be to generalize the existing patch. Create a comparison function that makes strong assumptions like word-aligned arguments, size of the arguments is multiple of word size, etc. Create a thin wrapper that asserts these under INVARIANTS.

Generic implementation might be just that, with specialized implementation in asm for amd64, for instance.

[mjg]

sys/netpfil/pf/pf.c

940–942

So I tried the flag in the context of the entirety of the kernel as opposed to an isolated small testcase and the compiler could not be convinced to stop emitting memcmp.

diff --git a/sys/conf/kern.mk b/sys/conf/kern.mk
index 35b7a66bab49..7971c4a87e45 100644
--- a/sys/conf/kern.mk
+++ b/sys/conf/kern.mk
@@ -142,6 +142,8 @@ INLINE_LIMIT?=      8000
 INLINE_LIMIT?= 8000
 .endif
 
+CFLAGS += -mllvm -max-loads-per-memcmp=16
+
 .if ${MACHINE_CPUARCH} == "aarch64"
 # We generally don't want fpu instructions in the kernel.
 CFLAGS += -mgeneral-regs-only

I verified this shows up on clang invocation line, no change in asm though:

[snip]
   0xffffffff8112bf45 <+165>:	data16 cs nopw 0x0(%rax,%rax,1)
   0xffffffff8112bf50 <+176>:	mov    $0x28,%edx
   0xffffffff8112bf55 <+181>:	mov    %r12,%rdi
   0xffffffff8112bf58 <+184>:	mov    %r15,%rsi
   0xffffffff8112bf5b <+187>:	call   0xffffffff81478ab0 <memcmp>
[snip]

[glebius]

sys/netpfil/pf/pf.c

940–942

That seems reasonable to me, assuming it won't break GCC builds. (And assuming we actually care about not breaking GCC builds?)

That would be CFLAGS.clang+=