From owner-dev-commits-src-all@freebsd.org Fri Sep 17 17:59:01 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id E58646B7172; Fri, 17 Sep 2021 17:59:01 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (glebi.us [162.251.186.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HB1sx293jz4pgm; Fri, 17 Sep 2021 17:59:01 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.16.1/8.16.1) with ESMTPS id 18HHwxVA039633 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Fri, 17 Sep 2021 10:58:59 -0700 (PDT) (envelope-from glebius@freebsd.org) Received: (from glebius@localhost) by cell.glebi.us (8.16.1/8.16.1/Submit) id 18HHwwqH039632; Fri, 17 Sep 2021 10:58:58 -0700 (PDT) (envelope-from glebius@freebsd.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@freebsd.org using -f Date: Fri, 17 Sep 2021 10:58:58 -0700 From: Gleb Smirnoff To: Mateusz Guzik 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: References: <202108171959.17HJx2lL069856@gitrepo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 4HB1sx293jz4pgm X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [0.00 / 15.00]; local_wl_from(0.00)[freebsd.org]; ASN(0.00)[asn:27348, ipnet:162.251.186.0/24, country:US] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Sep 2021 17:59:02 -0000 Mateusz, On Tue, Sep 14, 2021 at 05:11:11PM +0200, Mateusz Guzik wrote: M> > On Fri, Sep 10, 2021 at 09:43:06AM +0200, Mateusz Guzik wrote: M> > M> > M> diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c M> > M> > M> index e2dd3eb7c0de..add76c7b98d4 100644 M> > M> > M> --- a/sys/netpfil/pf/pf.c M> > M> > M> +++ b/sys/netpfil/pf/pf.c M> > M> > M> @@ -6151,7 +6151,7 @@ pf_test(int dir, int pflags, struct ifnet M> > *ifp, M> > M> > struct mbuf **m0, struct inpcb * M> > M> > M> M> > M> > M> PF_RULES_RLOCK(); M> > M> > M> M> > M> > M> - if (ip_divert_ptr != NULL && M> > M> > M> + if (__predict_false(ip_divert_ptr != NULL) && M> > M> > M> > M> > This is an optimization for a setup without divert(4) at cost of M> > M> > pessimization M> > M> > for a setup with divert(4). IMHO, __predict_false() predicate should M> > guard M> > M> > against error paths and other unusual events, not favor one setup over M> > M> > other. M> > M> > M> > M> M> > M> divert is non-standard and comes with tons of overhead, so I think M> > M> this is justified. M> > M> > pf is also non-standard and comes with tons of overhead, so what M> > about such change to ip_input(), and similar to ip_output(): M> > M> > diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c M> > index 465c00e4dac..e6feb837114 100644 M> > --- a/sys/netinet/ip_input.c M> > +++ b/sys/netinet/ip_input.c M> > @@ -605,7 +605,7 @@ ip_input(struct mbuf *m) M> > */ M> > M> > /* Jump over all PFIL processing if hooks are not active. */ M> > - if (!PFIL_HOOKED_IN(V_inet_pfil_head)) M> > + if (__predict_true(!PFIL_HOOKED_IN(V_inet_pfil_head))) M> > goto passin; M> > M> > odst = ip->ip_dst; M> > M> > I hope that brings my point. M> > M> > M> The real fix would be to either implement hot patchable branches or M> > M> otherwise generate pf_test always (or never) doing divert without M> > M> having to check for it, but that's far in the future. M> > M> > That would be great, but before we have such tools, I believe optimization M> > for one particular setup at cost of pessimization of other setups is not M> > a way to go. M> > M> M> So happens I would argue the pfil change should also be made, perhaps M> conditional on whether a known consumer is compiled in. Thanks for reply, although makes me think that you wouldn't reply anything unless you really want to proceed with my hyperbolic suggestion :( How deep are you going to go this path? May be if (__predict_true(ip->ip_proto == IPPROTO_TCP && th->th_port == ntohs(443))) A suggestion to get optimization for "non-standard" stuff by compiling it statically sounds like a jump 20 years to the past. M> I guess I should elaborate on how I see things here. M> M> The network stack comes with a rampant branch fest which can be M> significantly reduced in easy ways. For example from ip_input: M> M> #if defined(IPSEC) || defined(IPSEC_SUPPORT) M> /* M> * Bypass packet filtering for packets previously handled by IPsec. M> */ M> if (IPSEC_ENABLED(ipv4) && M> IPSEC_CAPS(ipv4, m, IPSEC_CAP_BYPASS_FILTER) != 0) M> goto passin; M> #endif M> M> Let's say this has to be checked every time. Even then IPSEC_CAPS is a M> func call which induces 2 more func calls, which also branch for no M> reason. Instead the complete result could be computed so that there is M> one bool to check (and even that could be hot patched later on). M> Finally, it should probably be predicted one way or the other. M> M> pf_test is an egregious example of this proble and, the commit at hand M> was a step towards cleaning the state up -- it is not meant to be M> permanent in the current form. The idea is to gradually split it into M> parts which have to be there and parts which are optional, the M> annotation will help going forward and should not measurably hurt M> divert. I understand your concerns, but this is the reality of network. A machine supports multiple protocols and encapsulations or whatever the same time. I understand, that some machines and workloads would work 99% of the time on just one protocol. So you create a benchmark that exercises a particular branch of code for all of the packets and then do optimizations for this particular branch. To justify such changes at least you need to have a second benchmark that would measure how much of pessimization you created for a setup that would have 99% prediction mismatch and document the results in the commit message, like: - improves packet rate without pf hooked in by X% - pessimizes packet rate with pf (empty ruleset) by Y% -- Gleb Smirnoff