Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 10 Sep 2021 11:32:51 -0700
From:      Gleb Smirnoff <glebius@freebsd.org>
To:        Mateusz Guzik <mjguzik@gmail.com>
Cc:        kp@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org
Subject:   Re: git: 5091ca26507b - main - pf: save on branching in the common case in pf_test
Message-ID:  <YTuk05n21r1bLEo1@FreeBSD.org>
In-Reply-To: <CAGudoHHrxk7=tYF66Q6DHFkwxmKb=QEKu%2BAYiDK9sfT5OGeqoQ@mail.gmail.com>
References:  <202108171959.17HJx2lL069856@gitrepo.freebsd.org> <YTrtUWbpWxxzviSQ@FreeBSD.org> <CAGudoHHrxk7=tYF66Q6DHFkwxmKb=QEKu%2BAYiDK9sfT5OGeqoQ@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
  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.

-- 
Gleb Smirnoff



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YTuk05n21r1bLEo1>