Message-ID: <CAFSKS=Nv-jObY1X8oCUSGoyYWP3csvNPyE-wUn6DAsDBWmKx5Q@mail.gmail.com>
Date:   Tue, 9 Feb 2021 13:09:40 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

On Tue, Feb 9, 2021 at 12:51 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Tue, Feb 09, 2021 at 12:37:38PM -0600, George McCollister wrote:
> > On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean <olteanv@...il.com> wrote:
> > >
> > > On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > > > > If you return zero, the software fallback is never going to kick in.
> > > >
> > > > For join and leave? How is this not a problem for the bridge and lag
> > > > functions? They work the same way don't they? I figured it would be
> > > > safe to follow what they were doing.
> > >
> > > I didn't say that the bridge and LAG offloading logic does the right
> > > thing, but it is on its way there...
> > >
> > > Those "XXX not offloaded" messages were tested with cases where the
> > > .port_lag_join callback _is_ present, but fails (due to things like
> > > incompatible xmit hashing policy). They were not tested with the case
> > > where the driver does not implement .port_lag_join at all.
> > >
> > > Doesn't mean you shouldn't do the right thing. I'll send some patches
> > > soon, hopefully, fixing that for LAG and the bridge, you can concentrate
> > > on HSR. For the non-offload scenario where the port is basically
> > > standalone, we also need to disable the other bridge functions such as
> > > address learning, otherwise it won't work properly, and that's where
> > > I've been focusing my attention lately. You can't offload the bridge in
> > > software, or a LAG, if you have address learning enabled. For HSR it's
> > > even more interesting, you need to have address learning disabled even
> > > when you offload the DANH/DANP.
> >
> > Do I just return -EOPNOTSUPP instead of 0 in dsa_switch_hsr_join and
> > dsa_switch_hsr_leave?
>
> Yes, return -EOPNOTSUPP if the callbacks are not implemented please.

Will do.

>
> > I'm not sure exactly what you're saying needs to be done wrt to
> > address learning with HSR. The switch does address learning
> > internally. Are you saying the DSA address learning needs to be
> > disabled?
>
> I'm saying that when you're doing any sort of redundancy protocol, the
> switch will get confused by address learning if it performs it at
> physical port level, because it will see the same source MAC address
> coming in from 2 (or more) different physical ports. And when it sees a
> packet coming in through a port that it had already learned it should be
> the destination for the MAC address because an earlier packet came
> having that MAC address as source, it will think it should do
> hairpinning which it's configured not to => it'll drop the packet.
>
> Now, your switch might have some sort of concept of address learning at
> logical port level, where the "logical port" would roughly correspond to
> the hsr0 upper (I haven't opened the XRS700x manual to know if it does
> this, sorry). Basically if you support RedBox I expect that the switch
> is able to learn at the level of "this MAC address came from the HSR
> ring, aka from one or both of the individual ring ports". But for
> configuring that in Linux, you'd need something like:

Yup, the switch has provisions to deal with this.

>
> ip link set hsr0 master br0
> ip link set hsr0 type bridge_slave learning on
>
> and then catch from DSA the switchdev notification emitted for hsr0, and
> use that to configure learning on the logical port of your switch
> corresponding to hsr0, instead of learning on the physical ports that
> offload it.
>
> There are similar issues related to address learning for everything
> except a bridge port, basically.
>
> > If that's something I need for this patch some tips on what
> > to do would be appreciated because I'm a bit lost.
>
> I didn't say you need to change something related to learning for this
> series, because you can't anyway - DSA doesn't give you the knobs to
> configure address learning yet. The series where I try to add those are
> here:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210209151936.97382-1-olteanv@gmail.com/
>
> The take-away is that with those changes, a DSA driver should start its
> ports in standalone mode with learning disabled and flooding of all
> kinds enabled, and then treat the .port_bridge_flags callback which
> should do the right thing and enable/disable address learning only when
> necessary.
>
> All I said is that address learning remaining enabled has been an issue
> that prevented the non-offload scenarios from really working, but you
> shouldn't be too hung up on that, and still return -EOPNOTSUPP, thereby
> allowing the software fallback to kick in, even if it doesn't work.

Thanks for clearing that up and explaining how this will work in the future.

Powered by blists - more mailing lists