Message-ID: <20220426185253.jiqfljymrwvyfteo@skbuf>
Date:   Tue, 26 Apr 2022 18:52:54 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        "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>,
        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>
Subject: Re: [PATCH net-next 2/5] net: dsa: add out-of-band tagging protocol

Hi Maxime,

On Tue, Apr 26, 2022 at 03:57:32PM +0200, Maxime Chevallier wrote:
> > First off, I am not a big fan of expanding skb::shared_info because
> > it is sensitive to cache line sizes and is critical for performance
> > at much higher speeds, I would expect Eric and Jakub to not be
> > terribly happy about it.
> 
> No problem, I'm testing with the skb->cb approach as you suggested and
> see how it goes.
> 
> > The Broadcom systemport (bcmsysport.c) has a mode where it can
> > extract the Broadcom tag and put it in front of the actual packet
> > contents which appears to be very similar here. From there on, you
> > can have two strategies:
> > 
> > - have the Ethernet controller mangle the packet contents such that
> > the QCA tag is located in front of the actual Ethernet frame and
> > create a new tagging protocol variant for QCA, similar to the
> > TAG_BRCM versus TAG_BRCM_PREPEND
> > 
> > - provide the necessary information for the tagger to work using an
> > out of band mechanism, which is what you have done, in which case,
> > maybe you can use skb->cb[] instead of using skb::shared_info?
> 
> One of the reason why I chose the second is to support possible future
> cases where another controller would face a similar situation, and also
> make use of the out-of-band tagger.
> 
> I understand that it's not very elegant in the sense that this breaks
> the nice tagging model we have, but adding/removing data before the
> payload also seems convoluted to achieve the same thing :) It seems
> that this approach comes with a bit of an overhead since it implies
> mangling the skb a bit, but I've yet to test this myself.
> 
> That's actually what I wanted your opinion on, it also seems like
> Andrew likes the idea of putting the tag ahead of the frame to stick
> with the actual model.
> 
> I don't have strong feelings myself on the way of doing this, I'm
> looking for an approach that is efficient but yet easily maintainable.

The skb->cb is not free to use for passing data between the DSA master
and the switch driver. There's the qdisc layer on TX, GRO on RX, maybe
others, and these mangle that memory region. So that would make it a -1
for skb->cb, and a +1 from my side for making the DSA master driver push
a fake prepended header, and consuming it "as usual" from the DSA
tagging protocol driver. That, plus the hope that I'll be long dead by
the time we'd need to find a solution that scales to more switches
designed like this :)

I'll take a closer look at your patches a bit later too, probably not today,
don't wait for my feedback if you think you're otherwise ready to repost.

Powered by blists - more mailing lists