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

From: Florian Fainelli
Date: Thu May 19 2022 - 13:11:24 EST



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@xxxxxxx> 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@xxxxxxxxxxx>
---

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/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