Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 11 Aug 2023 17:07:16 -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:  <CAK7dMtAYb1846-s6sVhLYe_UMjqZLTcUwc6ZVeZVbpEEj1dqtQ@mail.gmail.com>
In-Reply-To: <202308112251.37BMpOnC064624@gitrepo.freebsd.org>
References:  <202308112251.37BMpOnC064624@gitrepo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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.org> wr=
ote:
>
> The branch main has been updated by kp:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=3D5f11a33ceeb385477cb22d9ad5=
941061c5a26be9
>
> 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 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 on
> -        * VLANs, then guess it may do LRO on VLANs.  False positive here
> -        * cost nothing, while false negative may lead to some confusions=
.
> +       * 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 br=
eaks
> +       * the end-to-end principle and can significantly impact performan=
ce."
> +       *
> +       * 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 VLANs=
 then
>



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