Date: Sat, 12 Aug 2023 08:42:21 -0700 From: Kevin Bowling <kevin.bowling@kev009.com> To: Kristof Provost <kp@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 5f11a33ceeb3 - main - if_vlan: do not enable LRO for bridge interaces Message-ID: <CAK7dMtBhmc5Fu3M6oWNkdbiLErZSM0L=rwgtaV3Ht16249FFaA@mail.gmail.com> In-Reply-To: <F71B697E-9D8C-4518-A730-6794EAFD6495@FreeBSD.org> References: <202308112251.37BMpOnC064624@gitrepo.freebsd.org> <CAK7dMtAYb1846-s6sVhLYe_UMjqZLTcUwc6ZVeZVbpEEj1dqtQ@mail.gmail.com> <CAK7dMtACaWTXnygA1e0-YzP7YXWrGHuzM-ixLuf17EsMduym%2Bg@mail.gmail.com> <F71B697E-9D8C-4518-A730-6794EAFD6495@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Aug 12, 2023 at 1:45=E2=80=AFAM Kristof Provost <kp@freebsd.org> wr= ote: > > I also don=E2=80=99t have a setup to reproduce the issues Paul reported. > Given the discussion in the PR I assumed everyone was happy with the fix = he posted. Ok, I happened to be looking into it when the commit went in. Please revert, it's not the right fix. I have tested my patch as such: ifconfig bridge0 create ifconfig em3.11 create ifconfig bridge0 addm em3.11 You will see the em3.11 automagically has LRO removed and the "can't disable some capabilities" message is not present. ifconfig bridge0 deletem em3.11 LRO is restored to em3.11. My patch has the correct functionality intended by the masking code in if_bridge and my patch also fixes the case of disjoint interface capabilities in a bridge. The subtle bug was that vlan_capabilities() in if_vlan was not obeying the requested mask from its IFCAP ioctl. > Best regards, > Kristof Cheers, Kevin > On 12 Aug 2023, at 3:28, Kevin Bowling wrote: > > 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 VLAN= s then > > > > On Fri, Aug 11, 2023 at 5:07=E2=80=AFPM Kevin Bowling <kevin.bowling@ke= v009.com> 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 <kp@freebsd.or= g> wrote: > >>> > >>> The branch main has been updated by kp: > >>> > >>> URL: https://cgit.FreeBSD.org/src/commit/?id=3D5f11a33ceeb385477cb22d= 9ad5941061c5a26be9 > >>> > >>> commit 5f11a33ceeb385477cb22d9ad5941061c5a26be9 > >>> Author: Paul Vixie <paul@redbarn.org> > >>> AuthorDate: 2023-08-11 18:17:16 +0000 > >>> Commit: Kristof Provost <kp@FreeBSD.org> > >>> 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 brea= ks > >>> 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= on > >>> - * VLANs, then guess it may do LRO on VLANs. False positive = here > >>> - * cost nothing, while false negative may lead to some confus= ions. > >>> + * If the parent interface is not a bridge and can do LRO and > >>> + * checksum offloading on VLANs, then guess it may do LRO on V= LANs. > >>> + * 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 i= t breaks > >>> + * the end-to-end principle and can significantly impact perfo= rmance." > >>> + * > >>> + * 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 V= LANs then > >>>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAK7dMtBhmc5Fu3M6oWNkdbiLErZSM0L=rwgtaV3Ht16249FFaA>