On Thu, Sep 10, 2020 at 12:55:46PM -0700, Florian Fainelli wrote:
> 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 <olte...@gmail.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]
>
Uhm, it's right there? Not sure how you ended up with that warning.
And I know little about how branch prediction works, I thought it's
enough that the netdev_uses_dsa() check is already following an unlikely
path.
Thanks,
-Vladimir
