From owner-dev-commits-src-all@freebsd.org Tue Sep 14 15:11:14 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 7694966B32A; Tue, 14 Sep 2021 15:11:14 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lj1-x231.google.com (mail-lj1-x231.google.com [IPv6:2a00:1450:4864:20::231]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4H86Hj5Px7z3t5f; Tue, 14 Sep 2021 15:11:13 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lj1-x231.google.com with SMTP id p15so24549078ljn.3; Tue, 14 Sep 2021 08:11:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=nNYuDvZ7S81Nhz6iC5cEDTIYEweU8c+ABEPXgj568Lw=; b=mTRY4qTUVPVsocNO8GZvzfZ/pmnRfpPHA/xbUs1ui+Htb309aOMjSs5fIQ2GlHH+gN GOPReu4L03jtxDDbftXMYLgXkToWHIv7ihob3aSvlTSbaEeZv6Z5plrotRYliZt1vyq8 Cui3JYcyubR2wZfww0kcPXZ7tH7ZtBnoJ4HXf/pB9nt9raqCRu1F2FjEjIuG2zj1MfiI qOV+Q6At4RqbhamgAzOCvC2YAr8uQpfyYXFeOTOiPFa6dMqEMkHo/jkWvkQjJkwy/kTo ZpSSfS7vd6NybtiUo+UldAPMseHMDlSBRzDYl7J32+IQzkFM1N7xt7lsq90132H6sd+L ENUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=nNYuDvZ7S81Nhz6iC5cEDTIYEweU8c+ABEPXgj568Lw=; b=oPIlhsRRkEXJjNHujb+r5YajYFwi25pExOs3bi8iT/g32VdgJdKyXve3kV6FKQI9NU Vqxs5+nLd/26JcnE7ZxOJKfyGtILi7/iuW+AJRBS9oJp8Mr01Ubu23nr2nb70kmtNsO+ UQrxek+qxd6rGbTLW45M9qmwcBOEPbXB0sGxPHf7ieZSp4g9z4j7140c3fjsL7pjxuVH nlL4Fk+12afu8wLRx3Sfhf030LNeEt91oxkeS5GcxHdFJiVIlONoRgbJYXCxlSZgi6dN i8KLudodR9Y0uTRqCF7qWlRPwMVfZNh9c4qxslu1ol/3EcRZEIqeHr9wX9YMgLuL6o0T 8q5Q== X-Gm-Message-State: AOAM531GUNupjN6KKgAQq/Hfe0eZJW7JnK0bJ6uEvKji3R13HWic/bfu Jp4XGqqe5U2lu98bWsHz0HEmIZUMVb8AbxLvlHdE7AR3 X-Google-Smtp-Source: ABdhPJxAn5h9YS3ZWlq1mMJyCOpSf/3h+UWRYzerrYePrWIPmb9RvFa8sHYfl4Y0LdV7gagkYBD8qteb/5+rQszFszs= X-Received: by 2002:a2e:8906:: with SMTP id d6mr15908333lji.248.1631632272501; Tue, 14 Sep 2021 08:11:12 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:6f04:0:0:0:0:0 with HTTP; Tue, 14 Sep 2021 08:11:11 -0700 (PDT) In-Reply-To: References: <202108171959.17HJx2lL069856@gitrepo.freebsd.org> From: Mateusz Guzik Date: Tue, 14 Sep 2021 17:11:11 +0200 Message-ID: Subject: Re: git: 5091ca26507b - main - pf: save on branching in the common case in pf_test To: Gleb Smirnoff Cc: kp@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4H86Hj5Px7z3t5f X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=mTRY4qTU; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::231 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-4.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; RCVD_TLS_ALL(0.00)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36:c]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; RCPT_COUNT_FIVE(0.00)[5]; MID_RHS_MATCH_FROMTLD(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::231:from]; NEURAL_HAM_SHORT(-1.00)[-1.000]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; MAILMAN_DEST(0.00)[dev-commits-src-all,dev-commits-src-main]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] 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: Tue, 14 Sep 2021 15:11:14 -0000 On 9/10/21, Gleb Smirnoff 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