pf: merge pf_test() and pf_test6()

[kp]

Summary

Bye bye pf_test6(). Only one pf_test function for both IPv4 and v6.
The functions were 95% identical anyway.
OK bluhm@ mcbride@ and most probably henning@ as well

Obtained from: OpenBSD, claudio <[e-mail]>, c8bc4f6e29
Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

One suggestion you may like. There is never going to be a third version of IP in any nearest future. Note that sa_family_t refers to "socket address family", and there is no socket around. So I'd suggest to use internal const enum type for the AF_INET/AF_INET6 argument of pf_test() and all leaf functions that support two protocols. This will allow to remove all these panic("unknown af") clauses. Potentially that would allow compiler to optimize things, if some function implementations and calls are within a same compilation unit. To be fair I don't see any such optimizations opportunities with existing code.

[zlei]

In D46649#1063637, @glebius wrote:

One suggestion you may like. There is never going to be a third version of IP in any nearest future. Note that sa_family_t refers to "socket address family", and there is no socket around. So I'd suggest to use internal const enum type for the AF_INET/AF_INET6 argument of pf_test() and all leaf functions that support two protocols. This will allow to remove all these panic("unknown af") clauses. Potentially that would allow compiler to optimize things, if some function implementations and calls are within a same compilation unit. To be fair I don't see any such optimizations opportunities with existing code.

While I was reviewing the widely used kernel macro KASSERT / MPASS, I was wondering if the compiler can actually re-use this info, to optimize somewhat aggressively. To be precisely, when compile option DEBUG is turned off, make KASSERT / MPASS alias for __builtin_assume(). For the usage in this review, we can add KASSERT(af == AF_INET || af == AF_INET6) in top of pf_test(). So again panic("unknown af") clauses can be omitted.

Well that ( __builtin_assume ) is a big change and may beyond the scope of this review. I think that should be posted to #arch.

[mjg]

this can __assert_unreachable which will in production kernels will tell the compiler the 2 options are the only valid ones

[kp]

In D46649#1063637, @glebius wrote:

One suggestion you may like. There is never going to be a third version of IP in any nearest future. Note that sa_family_t refers to "socket address family", and there is no socket around. So I'd suggest to use internal const enum type for the AF_INET/AF_INET6 argument of pf_test() and all leaf functions that support two protocols. This will allow to remove all these panic("unknown af") clauses. Potentially that would allow compiler to optimize things, if some function implementations and calls are within a same compilation unit. To be fair I don't see any such optimizations opportunities with existing code.

I'm not sure I like defining a new enum for this, and converting between it and sa_family_t. It's likely have to spread pretty far in the pf code, or end up with conversions everywhere.
On the other hand, if we add the KASSERT(af == AF_INET || af == AF_INET6) zlei suggested we can also get rid of the extra panic() calls.

I'm also still importing more openbsd patches, and doing some other cleanup. I expect several of these switch(af) to go away later.

[kp]

In D46649#1063965, @kp wrote:

I'm not sure I like defining a new enum for this, and converting between it and sa_family_t. It's likely have to spread pretty far in the pf code, or end up with conversions everywhere.

Or maybe I do like it after all. clang and gcc seem happy to convert between the two if I define PF_AF_INET = AF_INET and PF_AF_INET6 = AF_INET6, which seems like a sensible thing to do.
I'll update this review when my test builds are done.

[zlei]

In D46649#1063973, @kp wrote:

In D46649#1063965, @kp wrote:

I'm not sure I like defining a new enum for this, and converting between it and sa_family_t. It's likely have to spread pretty far in the pf code, or end up with conversions everywhere.

Or maybe I do like it after all. clang and gcc seem happy to convert between the two if I define PF_AF_INET = AF_INET and PF_AF_INET6 = AF_INET6, which seems like a sensible thing to do.
I'll update this review when my test builds are done.

Since it ( sa_family_t ) is from upstream, I think it can be landed as is, and file a separate review for that. It will be easier to track the serials of those changes.

[kp]

In D46649#1063981, @zlei wrote:

Since it ( sa_family_t ) is from upstream, I think it can be landed as is, and file a separate review for that. It will be easier to track the serials of those changes.

That's in D46683