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 <[email protected]>
> ---
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?
Hi Vlad,
On Sat, 14 May 2022 22:40:03 +0000
Vladimir Oltean <[email protected]> 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 <[email protected]>
> > ---
>
> 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 ?
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.
Thanks,
Maxime
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 <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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).
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>
>>> 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
On Thu, May 19, 2022 at 10:11:13AM -0700, Florian Fainelli wrote:
> unless we somehow manage to put it in the linear portion of
> the SKB to avoid using any control buffer or extension.
But how? Essentially the DSA master has to look at a packet and
determine whether it came from DSA based on something which non-DSA
code could not have done. In fact, I'm looking at the calls to
skb_reset_mac_{header,len} from net/core/skbuff.c, specifically at VLAN
and MPLS, and I believe (but haven't tested) that pushing such headers
would also alter skb->mac_len to some value != ETH_HLEN. So simply
having the DSA master check whether DSA was there by checking whether
skb->mac_len is ETH_HLEN + DSA tag len could easily confuse DSA with
some other protocol of same header size.