Re: [PATCH net-next v2 2/5] net: dsa: add out-of-band tagging protocol

[Florian Fainelli <f.fainelli@gmail.com>]

On 5/19/2022 7:52 AM, Vladimir Oltean wrote:
> On Tue, May 17, 2022 at 09:01:56AM +0200, Maxime Chevallier wrote:
>> Hi Vlad,
>>
>> On Sat, 14 May 2022 22:40:03 +0000
>> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>
>>> On Sat, May 14, 2022 at 05:06:53PM +0200, Maxime Chevallier wrote:
>>>> This tagging protocol is designed for the situation where the link
>>>> between the MAC and the Switch is designed such that the Destination
>>>> Port, which is usually embedded in some part of the Ethernet
>>>> Header, is sent out-of-band, and isn't present at all in the
>>>> Ethernet frame.
>>>>
>>>> This can happen when the MAC and Switch are tightly integrated on an
>>>> SoC, as is the case with the Qualcomm IPQ4019 for example, where
>>>> the DSA tag is inserted directly into the DMA descriptors. In that
>>>> case, the MAC driver is responsible for sending the tag to the
>>>> switch using the out-of-band medium. To do so, the MAC driver needs
>>>> to have the information of the destination port for that skb.
>>>>
>>>> This out-of-band tagging protocol is using the very beggining of
>>>> the skb headroom to store the tag. The drawback of this approch is
>>>> that the headroom isn't initialized upon allocating it, therefore
>>>> we have a chance that the garbage data that lies there at
>>>> allocation time actually ressembles a valid oob tag. This is only
>>>> problematic if we are sending/receiving traffic on the master port,
>>>> which isn't a valid DSA use-case from the beggining. When dealing
>>>> from traffic to/from a slave port, then the oob tag will be
>>>> initialized properly by the tagger or the mac driver through the
>>>> use of the dsa_oob_tag_push() call.
>>>>
>>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>>> ---
>>>
>>> Why put the DSA pseudo-header at skb->head rather than push it using
>>> skb_push()? I thought you were going to check for the presence of a
>>> DSA header using something like skb->mac_len == ETH_HLEN + tag len,
>>> but right now it sounds like treating garbage in the headroom as a
>>> valid DSA tag is indeed a potential problem. If you can't sort that
>>> out using information from the header offsets alone, maybe an skb
>>> extension is required?
>>
>> Indeed, I thought of that, the main reason is that pushing/poping in
>> itself is not enough, you also have to move the whole mac_header to
>> leave room for the tag, and then re-set it in it's original location.
>> There's nothing wrong with this, but it looked a bit cumbersome just to
>> insert a dummy tag that gets removed rightaway. Does that make sense ?
> 
> You're thinking about inserting a header before the EtherType. But what
> has been said was to _prepend_ a header, i.e. put it before the Ethernet
> MAC DA. That way you don't need to move the Ethernet header.
> 
> But anyway, too much talk for mostly nothing, see below.
> 
>> But yes I would really like to get a way to know wether the tag is
>> there or not, I'll dig a bit more to see if I can find a way to get
>> this info from the various skb offsets in a reliable way.
> 
> Without an skb extension, this seems like an impossible task to me
> (which should also answer Florian's request for feedback on the proposal
> to share skb->cb with GRO, the qdisc, and whomever else there might be
> in the path between the DSA master and the switch).

Sorry I should have been clearer, the patch series that I pointed Maxime 
at earlier:

https://lore.kernel.org/lkml/1438322920.20182.144.camel@edumazet-glaptop2.roam.corp.google.com/T/

was initially accepted only to be reverted later on because on 64-bit 
host, there was not enough room in skb->cb[] to insert 4 bytes, so it 
got reverted.

So yes, I think we need to allocate a custom SKB extension if we want to 
convey the tag, unless we somehow manage to put it in the linear portion 
of the SKB to avoid using any control buffer or extension.
-- 
Florian