| Message ID | 20200910162218.1216347-1-olteanv@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] __netif_receive_skb_core: don't untag vlan from skb on DSA master | expand |
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@gmail.com> > --- > Resent, sorry, I forgot to copy the list. This looks fine to me, and the rationale makes sense, if you do resubmit, feel free to add: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
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@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? > check_vlan_id: > if (skb_vlan_tag_get_id(skb)) { > /* Vlan id is non 0 and vlan_do_receive() above couldn't >
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@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] > >> check_vlan_id: >> if (skb_vlan_tag_get_id(skb)) { >> /* Vlan id is non 0 and vlan_do_receive() above couldn't >> >
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 <olteanv@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
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)) { check_vlan_id: if (skb_vlan_tag_get_id(skb)) { /* Vlan id is non 0 and vlan_do_receive() above couldn't
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@gmail.com> --- Resent, sorry, I forgot to copy the list. net/core/dev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)