[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3ae92181-b80c-c152-4274-50711c3ed062@gmail.com>
Date: Thu, 10 Sep 2020 12:55:46 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Vladimir Oltean <olteanv@...il.com>, kuba@...nel.org,
andrew@...n.ch, willemdebruijn.kernel@...il.com,
edumazet@...gle.com
Cc: netdev@...r.kernel.org
Subject: Re: [RFC PATCH] __netif_receive_skb_core: don't untag vlan from skb
on DSA master
On 9/10/2020 12:46 PM, Florian Fainelli wrote:
>
>
> On 9/10/2020 9:22 AM, Vladimir Oltean wrote:
>> A DSA master interface has upper network devices, each representing an
>> Ethernet switch port attached to it. Demultiplexing the source ports and
>> setting skb->dev accordingly is done through the catch-all ETH_P_XDSA
>> packet_type handler. Catch-all because DSA vendors have various header
>> implementations, which can be placed anywhere in the frame: before the
>> DMAC, before the EtherType, before the FCS, etc. So, the ETH_P_XDSA
>> handler acts like an rx_handler more than anything.
>>
>> It is unlikely for the DSA master interface to have any other upper than
>> the DSA switch interfaces themselves. Only maybe a bridge upper*, but it
>> is very likely that the DSA master will have no 8021q upper. So
>> __netif_receive_skb_core() will try to untag the VLAN, despite the fact
>> that the DSA switch interface might have an 8021q upper. So the skb will
>> never reach that.
>>
>> So far, this hasn't been a problem because most of the possible
>> placements of the DSA switch header mentioned in the first paragraph
>> will displace the VLAN header when the DSA master receives the frame, so
>> __netif_receive_skb_core() will not actually execute any VLAN-specific
>> code for it. This only becomes a problem when the DSA switch header does
>> not displace the VLAN header (for example with a tail tag).
>>
>> What the patch does is it bypasses the untagging of the skb when there
>> is a DSA switch attached to this net device. So, DSA is the only
>> packet_type handler which requires seeing the VLAN header. Once skb->dev
>> will be changed, __netif_receive_skb_core() will be invoked again and
>> untagging, or delivery to an 8021q upper, will happen in the RX of the
>> DSA switch interface itself.
>>
>> *see commit 9eb8eff0cf2f ("net: bridge: allow enslaving some DSA master
>> network devices". This is actually the reason why I prefer keeping DSA
>> as a packet_type handler of ETH_P_XDSA rather than converting to an
>> rx_handler. Currently the rx_handler code doesn't support chaining, and
>> this is a problem because a DSA master might be bridged.
>>
>> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
>> ---
>> Resent, sorry, I forgot to copy the list.
>>
>> net/core/dev.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 152ad3b578de..952541ce1d9d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -98,6 +98,7 @@
>> #include <net/busy_poll.h>
>> #include <linux/rtnetlink.h>
>> #include <linux/stat.h>
>> +#include <net/dsa.h>
>> #include <net/dst.h>
>> #include <net/dst_metadata.h>
>> #include <net/pkt_sched.h>
>> @@ -5192,7 +5193,7 @@ static int __netif_receive_skb_core(struct
>> sk_buff **pskb, bool pfmemalloc,
>> }
>> }
>> - if (unlikely(skb_vlan_tag_present(skb))) {
>> + if (unlikely(skb_vlan_tag_present(skb)) &&
>> !netdev_uses_dsa(skb->dev)) {
>
> Not that I have performance numbers to claim this, but we would
> probably want:
>
> && likely(!netdev_uses_dsa(skb->dev))
>
> as well?
And #include <net/dsa.h> as it does not look like there is any implicit
header inclusion that provides that definition:
net/core/dev.c: In function '__netif_receive_skb_core':
net/core/dev.c:5196:46: error: implicit declaration of function
'netdev_uses_dsa'; did you mean 'netdev_reset_tc'?
[-Werror=implicit-function-declaration]
>
>> check_vlan_id:
>> if (skb_vlan_tag_get_id(skb)) {
>> /* Vlan id is non 0 and vlan_do_receive() above couldn't
>>
>
--
Florian
Powered by blists - more mailing lists