Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
@ 2019-06-19 16:01 YueHaibing
  2019-06-19 18:39 ` Stanislav Fomichev
  2019-06-22 23:19 ` [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect David Miller
  0 siblings, 2 replies; 18+ messages in thread
From: YueHaibing @ 2019-06-19 16:01 UTC (permalink / raw)
  To: davem, sdf, jianbol, jiri, mirq-linux, willemb
  Cc: linux-kernel, netdev, YueHaibing

We build vlan on top of bonding interface, which vlan offload
is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
BOND_XMIT_POLICY_ENCAP34.

__skb_flow_dissect() fails to get information from protocol headers
encapsulated within vlan, because 'nhoff' is points to IP header,
so bond hashing is based on layer 2 info, which fails to distribute
packets across slaves.

Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/core/flow_dissector.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 415b95f..2a52abb 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		    skb && skb_vlan_tag_present(skb)) {
 			proto = skb->protocol;
 		} else {
+			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
+				nhoff -=  sizeof(*vlan);
+
 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
 						    data, hlen, &_vlan);
 			if (!vlan) {
-- 
2.7.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-19 16:01 [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect YueHaibing
@ 2019-06-19 18:39 ` Stanislav Fomichev
  2019-06-20  7:20   ` Jiri Pirko
  2019-06-20 10:02   ` Yuehaibing
  2019-06-22 23:19 ` [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect David Miller
  1 sibling, 2 replies; 18+ messages in thread
From: Stanislav Fomichev @ 2019-06-19 18:39 UTC (permalink / raw)
  To: YueHaibing
  Cc: davem, sdf, jianbol, jiri, mirq-linux, willemb, linux-kernel, netdev

On 06/20, YueHaibing wrote:
> We build vlan on top of bonding interface, which vlan offload
> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
> BOND_XMIT_POLICY_ENCAP34.
> 
> __skb_flow_dissect() fails to get information from protocol headers
> encapsulated within vlan, because 'nhoff' is points to IP header,
> so bond hashing is based on layer 2 info, which fails to distribute
> packets across slaves.
> 
> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  net/core/flow_dissector.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 415b95f..2a52abb 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>  		    skb && skb_vlan_tag_present(skb)) {
>  			proto = skb->protocol;
>  		} else {
> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
> +				nhoff -=  sizeof(*vlan);
> +
Should we instead fix the place where the skb is allocated to properly
pull vlan (skb_vlan_untag)? I'm not sure this particular place is
supposed to work with an skb. Having an skb with nhoff pointing to
IP header but missing skb_vlan_tag_present() when with
proto==ETH_P_8021xx seems weird.

>  			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
>  						    data, hlen, &_vlan);
>  			if (!vlan) {
> -- 
> 2.7.0
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-19 18:39 ` Stanislav Fomichev
@ 2019-06-20  7:20   ` Jiri Pirko
  2019-06-20 10:02   ` Yuehaibing
  1 sibling, 0 replies; 18+ messages in thread
From: Jiri Pirko @ 2019-06-20  7:20 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: YueHaibing, davem, sdf, jianbol, jiri, mirq-linux, willemb,
	linux-kernel, netdev

Wed, Jun 19, 2019 at 08:39:38PM CEST, sdf@fomichev.me wrote:
>On 06/20, YueHaibing wrote:
>> We build vlan on top of bonding interface, which vlan offload
>> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> BOND_XMIT_POLICY_ENCAP34.
>> 
>> __skb_flow_dissect() fails to get information from protocol headers
>> encapsulated within vlan, because 'nhoff' is points to IP header,
>> so bond hashing is based on layer 2 info, which fails to distribute
>> packets across slaves.
>> 
>> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>  net/core/flow_dissector.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 415b95f..2a52abb 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>  		    skb && skb_vlan_tag_present(skb)) {
>>  			proto = skb->protocol;
>>  		} else {
>> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
>> +				nhoff -=  sizeof(*vlan);
>> +
>Should we instead fix the place where the skb is allocated to properly
>pull vlan (skb_vlan_untag)? I'm not sure this particular place is

Yes.

>supposed to work with an skb. Having an skb with nhoff pointing to
>IP header but missing skb_vlan_tag_present() when with
>proto==ETH_P_8021xx seems weird.
>
>>  			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
>>  						    data, hlen, &_vlan);
>>  			if (!vlan) {
>> -- 
>> 2.7.0
>> 
>> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-19 18:39 ` Stanislav Fomichev
  2019-06-20  7:20   ` Jiri Pirko
@ 2019-06-20 10:02   ` Yuehaibing
  2019-06-21  0:33     ` Stanislav Fomichev
  1 sibling, 1 reply; 18+ messages in thread
From: Yuehaibing @ 2019-06-20 10:02 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: davem, sdf, jianbol, jiri, mirq-linux, willemb, linux-kernel, netdev

On 2019/6/20 2:39, Stanislav Fomichev wrote:
> On 06/20, YueHaibing wrote:
>> We build vlan on top of bonding interface, which vlan offload
>> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> BOND_XMIT_POLICY_ENCAP34.
>>
>> __skb_flow_dissect() fails to get information from protocol headers
>> encapsulated within vlan, because 'nhoff' is points to IP header,
>> so bond hashing is based on layer 2 info, which fails to distribute
>> packets across slaves.
>>
>> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>  net/core/flow_dissector.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 415b95f..2a52abb 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>  		    skb && skb_vlan_tag_present(skb)) {
>>  			proto = skb->protocol;
>>  		} else {
>> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
>> +				nhoff -=  sizeof(*vlan);
>> +
> Should we instead fix the place where the skb is allocated to properly
> pull vlan (skb_vlan_untag)? I'm not sure this particular place is
> supposed to work with an skb. Having an skb with nhoff pointing to
> IP header but missing skb_vlan_tag_present() when with
> proto==ETH_P_8021xx seems weird.

The skb is a forwarded vxlan packet, it send through vlan interface like this:

   vlan_dev_hard_start_xmit
    --> __vlan_hwaccel_put_tag //vlan_tci and VLAN_TAG_PRESENT is set
    --> dev_queue_xmit
        --> validate_xmit_skb
          --> validate_xmit_vlan // vlan_hw_offload_capable is false
             --> __vlan_hwaccel_push_inside //here skb_push vlan_hlen, then clear skb->tci

    --> bond_start_xmit
       --> bond_xmit_hash
         --> __skb_flow_dissect // nhoff point to IP header
            -->  case htons(ETH_P_8021Q)
            // skb_vlan_tag_present is false, so
              vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), //vlan point to ip header wrongly

> 
>>  			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
>>  						    data, hlen, &_vlan);
>>  			if (!vlan) {
>> -- 
>> 2.7.0
>>
>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-20 10:02   ` Yuehaibing
@ 2019-06-21  0:33     ` Stanislav Fomichev
  2019-06-24 13:50       ` Jiri Pirko
  0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2019-06-21  0:33 UTC (permalink / raw)
  To: Yuehaibing
  Cc: davem, sdf, jianbol, jiri, mirq-linux, willemb, linux-kernel, netdev

On 06/20, Yuehaibing wrote:
> On 2019/6/20 2:39, Stanislav Fomichev wrote:
> > On 06/20, YueHaibing wrote:
> >> We build vlan on top of bonding interface, which vlan offload
> >> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
> >> BOND_XMIT_POLICY_ENCAP34.
> >>
> >> __skb_flow_dissect() fails to get information from protocol headers
> >> encapsulated within vlan, because 'nhoff' is points to IP header,
> >> so bond hashing is based on layer 2 info, which fails to distribute
> >> packets across slaves.
> >>
> >> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> >> ---
> >>  net/core/flow_dissector.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >> index 415b95f..2a52abb 100644
> >> --- a/net/core/flow_dissector.c
> >> +++ b/net/core/flow_dissector.c
> >> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >>  		    skb && skb_vlan_tag_present(skb)) {
> >>  			proto = skb->protocol;
> >>  		} else {
> >> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
> >> +				nhoff -=  sizeof(*vlan);
> >> +
> > Should we instead fix the place where the skb is allocated to properly
> > pull vlan (skb_vlan_untag)? I'm not sure this particular place is
> > supposed to work with an skb. Having an skb with nhoff pointing to
> > IP header but missing skb_vlan_tag_present() when with
> > proto==ETH_P_8021xx seems weird.
> 
> The skb is a forwarded vxlan packet, it send through vlan interface like this:
> 
>    vlan_dev_hard_start_xmit
>     --> __vlan_hwaccel_put_tag //vlan_tci and VLAN_TAG_PRESENT is set
>     --> dev_queue_xmit
>         --> validate_xmit_skb
>           --> validate_xmit_vlan // vlan_hw_offload_capable is false
>              --> __vlan_hwaccel_push_inside //here skb_push vlan_hlen, then clear skb->tci
> 
>     --> bond_start_xmit
>        --> bond_xmit_hash
>          --> __skb_flow_dissect // nhoff point to IP header
>             -->  case htons(ETH_P_8021Q)
>             // skb_vlan_tag_present is false, so
>               vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), //vlan point to ip header wrongly
I see, so bonding device propagates hw VLAN support from the slaves.
If one of the slaves doesn't have it, its disabled for the bond device.
Any idea why we do that? Why not pass skbs to the slave devices
instead and let them handle the hw/sw vlan implementation?
I see the propagation was added in 278339a42a1b 10 years ago and
I don't see any rationale in the commit description.
Somebody with more context should probably chime in :-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-19 16:01 [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect YueHaibing
  2019-06-19 18:39 ` Stanislav Fomichev
@ 2019-06-22 23:19 ` David Miller
  2019-06-24  3:49   ` [PATCH v2] " YueHaibing
  1 sibling, 1 reply; 18+ messages in thread
From: David Miller @ 2019-06-22 23:19 UTC (permalink / raw)
  To: yuehaibing; +Cc: sdf, jianbol, jiri, mirq-linux, willemb, linux-kernel, netdev

From: YueHaibing <yuehaibing@huawei.com>
Date: Thu, 20 Jun 2019 00:01:32 +0800

> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>  		    skb && skb_vlan_tag_present(skb)) {
>  			proto = skb->protocol;
>  		} else {
> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
> +				nhoff -=  sizeof(*vlan);

Even if this would have turned out to be the desired fix, you would need
to get rid of the extra spaces in that last statement.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-22 23:19 ` [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect David Miller
@ 2019-06-24  3:49   ` " YueHaibing
  2019-06-27  2:28     ` David Miller
  0 siblings, 1 reply; 18+ messages in thread
From: YueHaibing @ 2019-06-24  3:49 UTC (permalink / raw)
  To: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri
  Cc: linux-kernel, netdev, YueHaibing

We build vlan on top of bonding interface, which vlan offload
is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
BOND_XMIT_POLICY_ENCAP34.

__skb_flow_dissect() fails to get information from protocol headers
encapsulated within vlan, because 'nhoff' is points to IP header,
so bond hashing is based on layer 2 info, which fails to distribute
packets across slaves.

Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v2: remove redundant spaces
---
 net/core/flow_dissector.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 01ad60b..ff85934 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -998,6 +998,9 @@ bool __skb_flow_dissect(const struct net *net,
 		    skb && skb_vlan_tag_present(skb)) {
 			proto = skb->protocol;
 		} else {
+			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
+				nhoff -= sizeof(*vlan);
+
 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
 						    data, hlen, &_vlan);
 			if (!vlan) {
-- 
2.7.4



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-21  0:33     ` Stanislav Fomichev
@ 2019-06-24 13:50       ` Jiri Pirko
  2019-06-26  8:08         ` [PATCH] bonding: Always enable vlan tx offload YueHaibing
  2019-06-26 16:03         ` [PATCH] team: " YueHaibing
  0 siblings, 2 replies; 18+ messages in thread
From: Jiri Pirko @ 2019-06-24 13:50 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Yuehaibing, davem, sdf, jianbol, jiri, mirq-linux, willemb,
	linux-kernel, netdev

Fri, Jun 21, 2019 at 02:33:17AM CEST, sdf@fomichev.me wrote:
>On 06/20, Yuehaibing wrote:
>> On 2019/6/20 2:39, Stanislav Fomichev wrote:
>> > On 06/20, YueHaibing wrote:
>> >> We build vlan on top of bonding interface, which vlan offload
>> >> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> >> BOND_XMIT_POLICY_ENCAP34.
>> >>
>> >> __skb_flow_dissect() fails to get information from protocol headers
>> >> encapsulated within vlan, because 'nhoff' is points to IP header,
>> >> so bond hashing is based on layer 2 info, which fails to distribute
>> >> packets across slaves.
>> >>
>> >> Fixes: d5709f7ab776 ("flow_dissector: For stripped vlan, get vlan info from skb->vlan_tci")
>> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> >> ---
>> >>  net/core/flow_dissector.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> >> index 415b95f..2a52abb 100644
>> >> --- a/net/core/flow_dissector.c
>> >> +++ b/net/core/flow_dissector.c
>> >> @@ -785,6 +785,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >>  		    skb && skb_vlan_tag_present(skb)) {
>> >>  			proto = skb->protocol;
>> >>  		} else {
>> >> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
>> >> +				nhoff -=  sizeof(*vlan);
>> >> +
>> > Should we instead fix the place where the skb is allocated to properly
>> > pull vlan (skb_vlan_untag)? I'm not sure this particular place is
>> > supposed to work with an skb. Having an skb with nhoff pointing to
>> > IP header but missing skb_vlan_tag_present() when with
>> > proto==ETH_P_8021xx seems weird.
>> 
>> The skb is a forwarded vxlan packet, it send through vlan interface like this:
>> 
>>    vlan_dev_hard_start_xmit
>>     --> __vlan_hwaccel_put_tag //vlan_tci and VLAN_TAG_PRESENT is set
>>     --> dev_queue_xmit
>>         --> validate_xmit_skb
>>           --> validate_xmit_vlan // vlan_hw_offload_capable is false
>>              --> __vlan_hwaccel_push_inside //here skb_push vlan_hlen, then clear skb->tci
>> 
>>     --> bond_start_xmit
>>        --> bond_xmit_hash
>>          --> __skb_flow_dissect // nhoff point to IP header
>>             -->  case htons(ETH_P_8021Q)
>>             // skb_vlan_tag_present is false, so
>>               vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), //vlan point to ip header wrongly
>I see, so bonding device propagates hw VLAN support from the slaves.
>If one of the slaves doesn't have it, its disabled for the bond device.
>Any idea why we do that? Why not pass skbs to the slave devices
>instead and let them handle the hw/sw vlan implementation?

Probably due to historical reasons. It is indeed not needed to push the
vlan header. We should rather have the vlan_tci filled all the way down
to the transmitting netdevice. So the bonding/team should have the
NETIF_F_HW_VLAN_CTAG_TX and NETIF_F_HW_VLAN_STAG_TX flags always on.
That seems to be the correct fix to me.


>I see the propagation was added in 278339a42a1b 10 years ago and
>I don't see any rationale in the commit description.
>Somebody with more context should probably chime in :-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] bonding: Always enable vlan tx offload
  2019-06-24 13:50       ` Jiri Pirko
@ 2019-06-26  8:08         ` YueHaibing
  2019-06-26 15:25           ` Jiri Pirko
  2019-06-26 16:13           ` mirq-linux
  2019-06-26 16:03         ` [PATCH] team: " YueHaibing
  1 sibling, 2 replies; 18+ messages in thread
From: YueHaibing @ 2019-06-26  8:08 UTC (permalink / raw)
  To: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri,
	j.vosburgh, vfalico, andy
  Cc: linux-kernel, netdev, YueHaibing

We build vlan on top of bonding interface, which vlan offload
is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
BOND_XMIT_POLICY_ENCAP34.

Because vlan tx offload is off, vlan tci is cleared and skb push
the vlan header in validate_xmit_vlan() while sending from vlan
devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
get information from protocol headers encapsulated within vlan,
because 'nhoff' is points to IP header, so bond hashing is based
on layer 2 info, which fails to distribute packets across slaves.

This patch always enable bonding's vlan tx offload, pass the vlan
packets to the slave devices with vlan tci, let them to handle
vlan implementation.

Fixes: 278339a42a1b ("bonding: propogate vlan_features to bonding master")
Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 407f4095a37a..799fc38c5c34 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4320,12 +4320,12 @@ void bond_setup(struct net_device *bond_dev)
 	bond_dev->features |= NETIF_F_NETNS_LOCAL;
 
 	bond_dev->hw_features = BOND_VLAN_FEATURES |
-				NETIF_F_HW_VLAN_CTAG_TX |
 				NETIF_F_HW_VLAN_CTAG_RX |
 				NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
 	bond_dev->features |= bond_dev->hw_features;
+	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 }
 
 /* Destroy a bonding device.
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] bonding: Always enable vlan tx offload
  2019-06-26  8:08         ` [PATCH] bonding: Always enable vlan tx offload YueHaibing
@ 2019-06-26 15:25           ` Jiri Pirko
  2019-06-26 15:29             ` Yuehaibing
  2019-06-26 16:13           ` mirq-linux
  1 sibling, 1 reply; 18+ messages in thread
From: Jiri Pirko @ 2019-06-26 15:25 UTC (permalink / raw)
  To: YueHaibing
  Cc: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, j.vosburgh,
	vfalico, andy, linux-kernel, netdev

Wed, Jun 26, 2019 at 10:08:44AM CEST, yuehaibing@huawei.com wrote:
>We build vlan on top of bonding interface, which vlan offload
>is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>BOND_XMIT_POLICY_ENCAP34.
>
>Because vlan tx offload is off, vlan tci is cleared and skb push
>the vlan header in validate_xmit_vlan() while sending from vlan
>devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
>get information from protocol headers encapsulated within vlan,
>because 'nhoff' is points to IP header, so bond hashing is based
>on layer 2 info, which fails to distribute packets across slaves.
>
>This patch always enable bonding's vlan tx offload, pass the vlan
>packets to the slave devices with vlan tci, let them to handle
>vlan implementation.
>
>Fixes: 278339a42a1b ("bonding: propogate vlan_features to bonding master")
>Suggested-by: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Could you please do the same for team? Thanks!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] bonding: Always enable vlan tx offload
  2019-06-26 15:25           ` Jiri Pirko
@ 2019-06-26 15:29             ` Yuehaibing
  0 siblings, 0 replies; 18+ messages in thread
From: Yuehaibing @ 2019-06-26 15:29 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, j.vosburgh,
	vfalico, andy, linux-kernel, netdev

On 2019/6/26 23:25, Jiri Pirko wrote:
> Wed, Jun 26, 2019 at 10:08:44AM CEST, yuehaibing@huawei.com wrote:
>> We build vlan on top of bonding interface, which vlan offload
>> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> BOND_XMIT_POLICY_ENCAP34.
>>
>> Because vlan tx offload is off, vlan tci is cleared and skb push
>> the vlan header in validate_xmit_vlan() while sending from vlan
>> devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
>> get information from protocol headers encapsulated within vlan,
>> because 'nhoff' is points to IP header, so bond hashing is based
>> on layer 2 info, which fails to distribute packets across slaves.
>>
>> This patch always enable bonding's vlan tx offload, pass the vlan
>> packets to the slave devices with vlan tci, let them to handle
>> vlan implementation.
>>
>> Fixes: 278339a42a1b ("bonding: propogate vlan_features to bonding master")
>> Suggested-by: Jiri Pirko <jiri@resnulli.us>
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> 
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> 
> Could you please do the same for team? Thanks!

Sure, will send it, thank you!

> 
> .
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] team: Always enable vlan tx offload
  2019-06-24 13:50       ` Jiri Pirko
  2019-06-26  8:08         ` [PATCH] bonding: Always enable vlan tx offload YueHaibing
@ 2019-06-26 16:03         ` " YueHaibing
  2019-06-26 17:14           ` David Miller
  1 sibling, 1 reply; 18+ messages in thread
From: YueHaibing @ 2019-06-26 16:03 UTC (permalink / raw)
  To: davem, sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri,
	j.vosburgh, vfalico, andy
  Cc: linux-kernel, netdev, YueHaibing

We should rather have vlan_tci filled all the way down
to the transmitting netdevice and let it do the hw/sw
vlan implementation.

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/team/team.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b48006e7fa2f..a8bb25341bed 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2128,12 +2128,12 @@ static void team_setup(struct net_device *dev)
 	dev->features |= NETIF_F_NETNS_LOCAL;
 
 	dev->hw_features = TEAM_VLAN_FEATURES |
-			   NETIF_F_HW_VLAN_CTAG_TX |
 			   NETIF_F_HW_VLAN_CTAG_RX |
 			   NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
 	dev->features |= dev->hw_features;
+	dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 }
 
 static int team_newlink(struct net *src_net, struct net_device *dev,
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] bonding: Always enable vlan tx offload
  2019-06-26  8:08         ` [PATCH] bonding: Always enable vlan tx offload YueHaibing
  2019-06-26 15:25           ` Jiri Pirko
@ 2019-06-26 16:13           ` mirq-linux
  2019-06-26 16:48             ` Jiri Pirko
  1 sibling, 1 reply; 18+ messages in thread
From: mirq-linux @ 2019-06-26 16:13 UTC (permalink / raw)
  To: YueHaibing
  Cc: davem, sdf, jianbol, jiri, willemb, sdf, jiri, j.vosburgh,
	vfalico, andy, linux-kernel, netdev

On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
> We build vlan on top of bonding interface, which vlan offload
> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
> BOND_XMIT_POLICY_ENCAP34.
> 
> Because vlan tx offload is off, vlan tci is cleared and skb push
> the vlan header in validate_xmit_vlan() while sending from vlan
> devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
> get information from protocol headers encapsulated within vlan,
> because 'nhoff' is points to IP header, so bond hashing is based
> on layer 2 info, which fails to distribute packets across slaves.
> 
> This patch always enable bonding's vlan tx offload, pass the vlan
> packets to the slave devices with vlan tci, let them to handle
> vlan implementation.
[...]
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 407f4095a37a..799fc38c5c34 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4320,12 +4320,12 @@ void bond_setup(struct net_device *bond_dev)
>  	bond_dev->features |= NETIF_F_NETNS_LOCAL;
>  
>  	bond_dev->hw_features = BOND_VLAN_FEATURES |
> -				NETIF_F_HW_VLAN_CTAG_TX |
>  				NETIF_F_HW_VLAN_CTAG_RX |
>  				NETIF_F_HW_VLAN_CTAG_FILTER;
>  
>  	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>  	bond_dev->features |= bond_dev->hw_features;
> +	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
>  }
>  
>  /* Destroy a bonding device.
> 

I can see that bonding driver uses dev_queue_xmit() to pass packets to
slave links, but I can't see where in the path it does software fallback
for devices without HW VLAN tagging. Generally drivers that don't ever
do VLAN offload also ignore vlan_tci presence. Am I missing something
here?

Best Regards,
Micha³ Miros³aw

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] bonding: Always enable vlan tx offload
  2019-06-26 16:13           ` mirq-linux
@ 2019-06-26 16:48             ` Jiri Pirko
  2019-06-26 23:29               ` Michał Mirosław
  0 siblings, 1 reply; 18+ messages in thread
From: Jiri Pirko @ 2019-06-26 16:48 UTC (permalink / raw)
  To: mirq-linux
  Cc: YueHaibing, davem, sdf, jianbol, jiri, willemb, sdf, j.vosburgh,
	vfalico, andy, linux-kernel, netdev

Wed, Jun 26, 2019 at 06:13:38PM CEST, mirq-linux@rere.qmqm.pl wrote:
>On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
>> We build vlan on top of bonding interface, which vlan offload
>> is off, bond mode is 802.3ad (LACP) and xmit_hash_policy is
>> BOND_XMIT_POLICY_ENCAP34.
>> 
>> Because vlan tx offload is off, vlan tci is cleared and skb push
>> the vlan header in validate_xmit_vlan() while sending from vlan
>> devices. Then in bond_xmit_hash, __skb_flow_dissect() fails to
>> get information from protocol headers encapsulated within vlan,
>> because 'nhoff' is points to IP header, so bond hashing is based
>> on layer 2 info, which fails to distribute packets across slaves.
>> 
>> This patch always enable bonding's vlan tx offload, pass the vlan
>> packets to the slave devices with vlan tci, let them to handle
>> vlan implementation.
>[...]
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 407f4095a37a..799fc38c5c34 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4320,12 +4320,12 @@ void bond_setup(struct net_device *bond_dev)
>>  	bond_dev->features |= NETIF_F_NETNS_LOCAL;
>>  
>>  	bond_dev->hw_features = BOND_VLAN_FEATURES |
>> -				NETIF_F_HW_VLAN_CTAG_TX |
>>  				NETIF_F_HW_VLAN_CTAG_RX |
>>  				NETIF_F_HW_VLAN_CTAG_FILTER;
>>  
>>  	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>>  	bond_dev->features |= bond_dev->hw_features;
>> +	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
>>  }
>>  
>>  /* Destroy a bonding device.
>> 
>
>I can see that bonding driver uses dev_queue_xmit() to pass packets to
>slave links, but I can't see where in the path it does software fallback
>for devices without HW VLAN tagging. Generally drivers that don't ever
>do VLAN offload also ignore vlan_tci presence. Am I missing something
>here?

validate_xmit_skb->validate_xmit_vlan


>
>Best Regards,
>Michał Mirosław

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] team: Always enable vlan tx offload
  2019-06-26 16:03         ` [PATCH] team: " YueHaibing
@ 2019-06-26 17:14           ` David Miller
  0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2019-06-26 17:14 UTC (permalink / raw)
  To: yuehaibing
  Cc: sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri, j.vosburgh,
	vfalico, andy, linux-kernel, netdev

From: YueHaibing <yuehaibing@huawei.com>
Date: Thu, 27 Jun 2019 00:03:39 +0800

> We should rather have vlan_tci filled all the way down
> to the transmitting netdevice and let it do the hw/sw
> vlan implementation.
> 
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] bonding: Always enable vlan tx offload
  2019-06-26 16:48             ` Jiri Pirko
@ 2019-06-26 23:29               ` Michał Mirosław
  0 siblings, 0 replies; 18+ messages in thread
From: Michał Mirosław @ 2019-06-26 23:29 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: YueHaibing, netdev

On Wed, Jun 26, 2019 at 06:48:53PM +0200, Jiri Pirko wrote:
> Wed, Jun 26, 2019 at 06:13:38PM CEST, mirq-linux@rere.qmqm.pl wrote:
> >On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
[...]
> >> This patch always enable bonding's vlan tx offload, pass the vlan
> >> packets to the slave devices with vlan tci, let them to handle
> >> vlan implementation.
[...]
> >I can see that bonding driver uses dev_queue_xmit() to pass packets to
> >slave links, but I can't see where in the path it does software fallback
> >for devices without HW VLAN tagging. Generally drivers that don't ever
> >do VLAN offload also ignore vlan_tci presence. Am I missing something
> >here?

> validate_xmit_skb->validate_xmit_vlan

Yes it is. Thanks!

Best Regards,
Micha³ Miros³aw

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-24  3:49   ` [PATCH v2] " YueHaibing
@ 2019-06-27  2:28     ` David Miller
  2019-06-27  7:44       ` Jiri Pirko
  0 siblings, 1 reply; 18+ messages in thread
From: David Miller @ 2019-06-27  2:28 UTC (permalink / raw)
  To: yuehaibing
  Cc: sdf, jianbol, jiri, mirq-linux, willemb, sdf, jiri, linux-kernel, netdev

From: YueHaibing <yuehaibing@huawei.com>
Date: Mon, 24 Jun 2019 11:49:13 +0800

> @@ -998,6 +998,9 @@ bool __skb_flow_dissect(const struct net *net,
>  		    skb && skb_vlan_tag_present(skb)) {
>  			proto = skb->protocol;
>  		} else {
> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
> +				nhoff -= sizeof(*vlan);
> +

But this is wrong when we are being called via eth_get_headlen(), in
that case nhoff will be sizeof(struct ethhdr).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] flow_dissector: Fix vlan header offset in __skb_flow_dissect
  2019-06-27  2:28     ` David Miller
@ 2019-06-27  7:44       ` Jiri Pirko
  0 siblings, 0 replies; 18+ messages in thread
From: Jiri Pirko @ 2019-06-27  7:44 UTC (permalink / raw)
  To: David Miller
  Cc: yuehaibing, sdf, jianbol, jiri, mirq-linux, willemb, sdf,
	linux-kernel, netdev

Thu, Jun 27, 2019 at 04:28:29AM CEST, davem@davemloft.net wrote:
>From: YueHaibing <yuehaibing@huawei.com>
>Date: Mon, 24 Jun 2019 11:49:13 +0800
>
>> @@ -998,6 +998,9 @@ bool __skb_flow_dissect(const struct net *net,
>>  		    skb && skb_vlan_tag_present(skb)) {
>>  			proto = skb->protocol;
>>  		} else {
>> +			if (dissector_vlan == FLOW_DISSECTOR_KEY_MAX)
>> +				nhoff -= sizeof(*vlan);
>> +
>
>But this is wrong when we are being called via eth_get_headlen(), in
>that case nhoff will be sizeof(struct ethhdr).

This patch was replaced by:
[PATCH] bonding: Always enable vlan tx offload
http://patchwork.ozlabs.org/patch/1122886/

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 16:01 [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect YueHaibing
2019-06-19 18:39 ` Stanislav Fomichev
2019-06-20  7:20   ` Jiri Pirko
2019-06-20 10:02   ` Yuehaibing
2019-06-21  0:33     ` Stanislav Fomichev
2019-06-24 13:50       ` Jiri Pirko
2019-06-26  8:08         ` [PATCH] bonding: Always enable vlan tx offload YueHaibing
2019-06-26 15:25           ` Jiri Pirko
2019-06-26 15:29             ` Yuehaibing
2019-06-26 16:13           ` mirq-linux
2019-06-26 16:48             ` Jiri Pirko
2019-06-26 23:29               ` Michał Mirosław
2019-06-26 16:03         ` [PATCH] team: " YueHaibing
2019-06-26 17:14           ` David Miller
2019-06-22 23:19 ` [PATCH] flow_dissector: Fix vlan header offset in __skb_flow_dissect David Miller
2019-06-24  3:49   ` [PATCH v2] " YueHaibing
2019-06-27  2:28     ` David Miller
2019-06-27  7:44       ` Jiri Pirko

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox