git: 5091ca26507b - main - pf: save on branching in the common case in pf_test

[Mateusz Guzik]

On 9/10/21, Gleb Smirnoff <glebius at freebsd.org> wrote:
>   Mateusz,
>
> On Fri, Sep 10, 2021 at 09:43:06AM +0200, Mateusz Guzik wrote:
> M> > M> diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
> M> > M> index e2dd3eb7c0de..add76c7b98d4 100644
> M> > M> --- a/sys/netpfil/pf/pf.c
> M> > M> +++ b/sys/netpfil/pf/pf.c
> M> > M> @@ -6151,7 +6151,7 @@ pf_test(int dir, int pflags, struct ifnet
> *ifp,
> M> > struct mbuf **m0, struct inpcb *
> M> > M>
> M> > M>  	PF_RULES_RLOCK();
> M> > M>
> M> > M> -	if (ip_divert_ptr != NULL &&
> M> > M> +	if (__predict_false(ip_divert_ptr != NULL) &&
> M> >
> M> > This is an optimization for a setup without divert(4) at cost of
> M> > pessimization
> M> > for a setup with divert(4). IMHO, __predict_false() predicate should
> guard
> M> > against error paths and other unusual events, not favor one setup over
> M> > other.
> M> >
> M>
> M> divert is non-standard and comes with tons of overhead, so I think
> M> this is justified.
>
> pf is also non-standard and comes with tons of overhead, so what
> about such change to ip_input(), and similar to ip_output():
>
> diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
> index 465c00e4dac..e6feb837114 100644
> --- a/sys/netinet/ip_input.c
> +++ b/sys/netinet/ip_input.c
> @@ -605,7 +605,7 @@ ip_input(struct mbuf *m)
>          */
>
>         /* Jump over all PFIL processing if hooks are not active. */
> -       if (!PFIL_HOOKED_IN(V_inet_pfil_head))
> +       if (__predict_true(!PFIL_HOOKED_IN(V_inet_pfil_head)))
>                 goto passin;
>
>         odst = ip->ip_dst;
>
> I hope that brings my point.
>
> M> The real fix would be to either implement hot patchable branches or
> M> otherwise generate pf_test always (or never) doing divert without
> M> having to check for it, but that's far in the future.
>
> That would be great, but before we have such tools, I believe optimization
> for one particular setup at cost of pessimization of other setups is not
> a way to go.
>

So happens I would argue the pfil change should also be made, perhaps
conditional on whether a known consumer is compiled in.

I guess I should elaborate on how I see things here.

The network stack comes with a rampant branch fest which can be
significantly reduced in easy ways. For example from ip_input:

#if defined(IPSEC) || defined(IPSEC_SUPPORT)
        /*
         * Bypass packet filtering for packets previously handled by IPsec.
         */
        if (IPSEC_ENABLED(ipv4) &&
            IPSEC_CAPS(ipv4, m, IPSEC_CAP_BYPASS_FILTER) != 0)
                        goto passin;
#endif

Let's say this has to be checked every time. Even then IPSEC_CAPS is a
func call which induces 2 more func calls, which also branch for no
reason. Instead the complete result could be computed so that there is
one bool to check (and even that could be hot patched later on).
Finally, it should probably be predicted one way or the other.

pf_test is an egregious example of this proble and, the commit at hand
was a step towards cleaning the state up -- it is not meant to be
permanent in the current form. The idea is to gradually split it into
parts which have to be there and parts which are optional, the
annotation will help going forward and should not measurably hurt
divert.

-- 
Mateusz Guzik <mjguzik gmail.com>