From nobody Sat Aug 12 01:28:32 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RN3321gGjz4q47d for ; Sat, 12 Aug 2023 01:28:46 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: from mail-oa1-x2b.google.com (mail-oa1-x2b.google.com [IPv6:2001:4860:4864:20::2b]) (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 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RN3312vCYz4TnT for ; Sat, 12 Aug 2023 01:28:45 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Authentication-Results: mx1.freebsd.org; dkim=none ("invalid DKIM record") header.d=kev009.com header.s=google header.b=sKqw+yJu; spf=pass (mx1.freebsd.org: domain of kevin.bowling@kev009.com designates 2001:4860:4864:20::2b as permitted sender) smtp.mailfrom=kevin.bowling@kev009.com; dmarc=none Received: by mail-oa1-x2b.google.com with SMTP id 586e51a60fabf-1bfca0ec8b9so2198406fac.2 for ; Fri, 11 Aug 2023 18:28:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kev009.com; s=google; t=1691803724; x=1692408524; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=kB1TNYi94MlDzT6NjhbL3bNKM27jyM9fIEnj6rc2g8E=; b=sKqw+yJuTybBCLiQxaYZUWfdNzLZjfGZHynYQY+5hSCrtTNRut2WRrNqkm/3f+gvcp TRx0IJnUxbKdfM0E8iALJ/AUxhcPDnqBJgrXgHgneeZIlVng/562LAyPTlU0Oabdl2dq GfqrFPCY3KEqC2WBwOLBGOBdT7xXnsEZ5z5tY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691803724; x=1692408524; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=kB1TNYi94MlDzT6NjhbL3bNKM27jyM9fIEnj6rc2g8E=; b=NFq/rVguGY0OhHl/0CNRsRiChfiC+fn7vEdzDN+7R1FVO4exBenFF2hU7XZR8A97U0 5aMH++yUKa249rp62A4VK8yrM+vS8D0jh79kEw7bvOzy0rHygEwzyMpQbDpDCUBxJtPC MgNirITHOtXwHcYqvYL+LVftqGGAZJLI/eiXaCz4dL4wMFJTW0216X1/bPyJNr7jUOqy TamD5tjh5Z3UD9y4d2aFapQqXkkImmf4bbCFRxU0HGWn2n4dBTySLPFbFtObzqHZXXRk hKOwM14N0pDBG56HNANR7gR6B30Abt03wIGuMQKFkFsu9Uewo9VUMaQ6ldOfoTxBGtmd AJTA== X-Gm-Message-State: AOJu0YzHDbJ1njVCyzC0joYpVbC46qnbuOiwzW56ZD8FQpGJM5KDBJGp 2CaLkS5F8jPUDvTbzezKgRf+T62BwxCsQxfo4uadFA== X-Google-Smtp-Source: AGHT+IEAGFTCUkaiTUdYNTdXmEMN5Z50Di9LFYj6qetTmWTYqXDZdfenZNn3OYZ+UAehjRauEH+V689ci2rJ1kETlHU= X-Received: by 2002:a05:6870:9629:b0:1bb:bcc3:c96 with SMTP id d41-20020a056870962900b001bbbcc30c96mr3619246oaq.33.1691803723711; Fri, 11 Aug 2023 18:28:43 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202308112251.37BMpOnC064624@gitrepo.freebsd.org> In-Reply-To: From: Kevin Bowling Date: Fri, 11 Aug 2023 18:28:32 -0700 Message-ID: Subject: Re: git: 5f11a33ceeb3 - main - if_vlan: do not enable LRO for bridge interaces To: Kristof Provost Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spamd-Result: default: False [-3.27 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; NEURAL_HAM_LONG(-0.97)[-0.968]; R_SPF_ALLOW(-0.20)[+ip6:2001:4860:4000::/36]; MIME_GOOD(-0.10)[text/plain]; RCVD_COUNT_ONE(0.00)[1]; R_DKIM_PERMFAIL(0.00)[kev009.com:s=google]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org]; FROM_EQ_ENVFROM(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[2001:4860:4864:20::2b:from]; MIME_TRACE(0.00)[0:+]; ARC_NA(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_TLS_LAST(0.00)[]; TO_DN_SOME(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; DKIM_TRACE(0.00)[kev009.com:~]; FROM_HAS_DN(0.00)[]; DMARC_NA(0.00)[kev009.com]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-all@freebsd.org]; ASN(0.00)[asn:15169, ipnet:2001:4860:4864::/48, country:US] X-Spamd-Bar: --- X-Rspamd-Queue-Id: 4RN3312vCYz4TnT I think this may be a better fix, currently untested diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c index 6aa872a19364..51f0bc63ebda 100644 --- a/sys/net/if_vlan.c +++ b/sys/net/if_vlan.c @@ -2074,7 +2074,7 @@ vlan_capabilities(struct ifvlan *ifv) if (p->if_capabilities & IFCAP_VLAN_HWCSUM) cap |=3D p->if_capabilities & IFCAP_LRO; if (p->if_capenable & IFCAP_VLAN_HWCSUM) - ena |=3D p->if_capenable & IFCAP_LRO; + ena |=3D mena & IFCAP_LRO; /* * If the parent interface can offload TCP connections over VLANs th= en On Fri, Aug 11, 2023 at 5:07=E2=80=AFPM Kevin Bowling wrote: > > I am wondering what is going on with this patch, it doesn't seem like > a full fix. At the very least we should clean up the "can't disable > some capabilities" message. > > I've been looking at this for a couple hours and am wondering what is > going on in if_bridge.. it looks like it correctly loops over the > interfaces and masks ifcaps it doesn't like using the interface's > ioctl SIOCSIFCAP. A similar pattern is used in if_lagg. I don't see > any issue there so far. I also don't see the SIOCSICAP ioctl failing > nor did vixie's message report that. > > I checked on a cxgbe(9) bridged vlan setup I have and see this message > "bridge0: can't disable some capabilities on vlan0: 0x400" > > So I am wondering if the bug is that if_vlan SIOCSIFCAP is > disregarding the ioctl's request mask.. it is just blindly calling > vlan_capabilities. > > Regards, > Kevin > > > On Fri, Aug 11, 2023 at 3:51=E2=80=AFPM Kristof Provost = wrote: > > > > The branch main has been updated by kp: > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=3D5f11a33ceeb385477cb22d9a= d5941061c5a26be9 > > > > commit 5f11a33ceeb385477cb22d9ad5941061c5a26be9 > > Author: Paul Vixie > > AuthorDate: 2023-08-11 18:17:16 +0000 > > Commit: Kristof Provost > > CommitDate: 2023-08-11 22:50:37 +0000 > > > > if_vlan: do not enable LRO for bridge interaces > > > > If the parent interface is not a bridge and can do LRO and > > checksum offloading on VLANs, then guess it may do LRO on VLANs. > > False positive here cost nothing, while false negative may lead > > to some confusions. According to Wikipedia: > > > > "LRO should not operate on machines acting as routers, as it breaks > > the end-to-end principle and can significantly impact performance." > > > > The same reasoning applies to machines acting as bridges. > > > > PR: 254596 > > MFC after: 3 weeks > > --- > > sys/net/if_vlan.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c > > index 6aa872a19364..92e4e4247e3d 100644 > > --- a/sys/net/if_vlan.c > > +++ b/sys/net/if_vlan.c > > @@ -2067,14 +2067,22 @@ vlan_capabilities(struct ifvlan *ifv) > > } > > > > /* > > - * If the parent interface can do LRO and checksum offloading o= n > > - * VLANs, then guess it may do LRO on VLANs. False positive he= re > > - * cost nothing, while false negative may lead to some confusio= ns. > > + * If the parent interface is not a bridge and can do LRO and > > + * checksum offloading on VLANs, then guess it may do LRO on VLA= Ns. > > + * False positive here cost nothing, while false negative may le= ad > > + * to some confusions. According to Wikipedia: > > + * > > + * "LRO should not operate on machines acting as routers, as it = breaks > > + * the end-to-end principle and can significantly impact perform= ance." > > + * > > + * The same reasoning applies to machines acting as bridges. > > */ > > - if (p->if_capabilities & IFCAP_VLAN_HWCSUM) > > - cap |=3D p->if_capabilities & IFCAP_LRO; > > - if (p->if_capenable & IFCAP_VLAN_HWCSUM) > > - ena |=3D p->if_capenable & IFCAP_LRO; > > + if (ifp->if_bridge =3D=3D NULL) { > > + if (p->if_capabilities & IFCAP_VLAN_HWCSUM) > > + cap |=3D p->if_capabilities & IFCAP_LRO; > > + if (p->if_capenable & IFCAP_VLAN_HWCSUM) > > + ena |=3D p->if_capenable & IFCAP_LRO; > > + } > > > > /* > > * If the parent interface can offload TCP connections over VLA= Ns then > >