Message-ID: <20221107172846.y5nmi3plzd4wemmv@skbuf>
Date:   Mon, 7 Nov 2022 17:28:46 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Maxime Chevallier <maxime.chevallier@...tlin.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "thomas.petazzoni@...tlin.com" <thomas.petazzoni@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Luka Perkov <luka.perkov@...tura.hr>,
        Robert Marko <robert.marko@...tura.hr>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>
Subject: Re: [PATCH net-next v8 3/5] net: dsa: add out-of-band tagging
 protocol

On Mon, Nov 07, 2022 at 08:45:35AM -0800, Jakub Kicinski wrote:
> On Mon, 7 Nov 2022 11:27:37 +0000 Vladimir Oltean wrote:
> > Since this is the model that skb extensions propose and not something
> > that Maxime invented for this series, I presume that's not such a big
> > deal?
> 
> It's not a generic "do whatever you want" with it feature. The more
> people use it the less possible it is to have it disabled in a host-
> -centric kernel. 

We were talking about "the model" being "the model where you allocate
the extension for each packet", no?

> > What's more, couldn't this specific limitation of skb extensions
> > be addressed in a punctual way, via one-time calls to __skb_ext_alloc()
> > and fast path calls to __skb_ext_set()?
> 
> Are you suggesting we add refcounting to the skb ext?

idk, is it such a big offence? :)

Actually my previous paragraph on which you replied with an apparently
unrelated comment was saying that I think we're okay with allocating an
skb extension for each packet, if that's what the skb extension usage
model proposes.

> > I'm unfamiliar to the concept of destination cache entries and even more
> > so to the concept of struct dst_entry * carrying metadata. I suppose the
> > latter were introduced for lack of space in struct sk_buff, to carry
> > metadata between layers that aren't L3/L4 (where normal dst_entry structs
> > are used)? What makes metadata dst's preferable to skb extensions?
> 
> It's much less invasive.

Don't get me wrong, I don't oppose a dst_metadata solution as long as I
think that I understand it and that I can maintain/extend it as needed
going forward (which I clearly think I do about skb extensions, they
seem simple to use to the naive reader). No need to get territorial
about it, better to arm yourself with a bit of patience.

> 
> > The latter are more general; AFAIK they can be used between any layer
> > and any other layer, like for example between RX and TX in the
> > forwarding path.
> 
> You can't be using lower-dev / upper-dev metadata across forwarding,
> how would that ever work?

What makes metadata dst's preferable to skb extensions?
           ~~~~~~~~~~~~                 ~~~~~~~~~~~~~~
           former                       latter

I said: "The latter [aka skb extensions, not metadata dst's] are more general".
I did not say that you can use metadata dst's across forwarding, quite
the opposite.

> 
> > Side note, I am not exactly clear what are the lifetime
> > guarantees of a metadata dst entry, and if DSA's use would be 100% safe
> > (DSA is kind of L3, since it has an ETH_P_XDSA packet_type handler, not
> > an rx_handler).
> 
> It's just a refcounted object. I presume the DSA uppers can't get
> spawned before the lower is spawned already?

By lifetime guarantees, I actually meant: what is the latest point
during the RX path that skb_dst() will still point to the metadata dst,
and not get replaced with the real destination cache entry?

I think we're okay, because although DSA presents itself as an L3
protocol in the RX path, the 'real' L3 protocol handler will surely not
have run earlier than DSA, due to how eth_type_trans() was patched to
return ETH_P_XDSA.

Or I might be reading things completely wrong. Again, I have no
experience with destination cache entry structures or their metadata
carrying kind. Or with skb extensions, for that matter, other than
noticing that they exist.

> 
> > More importantly, what happens if a DSA switch is used together with a
> > SRIOV-capable DSA master which already uses METADATA_HW_PORT_MUX for
> > PF-VF communication? (if I understood the commit message of 3fcece12bc1b
> > ("net: store port/representator id in metadata_dst") correctly)
> 
> Let's be clear that the OOB metadata model only works if both upper and 
> lower are aware of the metadata. In fact they are pretty tightly bound.
> So chances of a mismatch are extremely low and theorizing about them is
> academic.

Legally I'm not allowed to say too much, but let's say I've heard about
something which makes the above not theoretical. Anyway, let's assume
it's not a concern.

> 
> In general, I'm not sure if pretending this is DSA is not an unnecessary
> complication which will end up hurting both ends of the equation.

This is a valid point. We've refused wacky "not DSA, not switchdev"
hardware before:
https://lore.kernel.org/netdev/20201125232459.378-1-lukma@denx.de/
There's also the option of doing what I did with ocelot/felix: a common
switch lib and 2 distinct front-ends, one switchdev and one DSA.

Not a lot of people seem to be willing to put that effort in, though.
The imx28 patch set was eventually abandoned. I though I'd try a
different approach this time. Idk, maybe it's a waste of time.

Powered by blists - more mailing lists