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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
From: Stanislav Fomichev <sdf@fomichev.me> Date: 2019-06-19 18:39:42
On 06/20, YueHaibing wrote:
quoted hunk
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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
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.
Wed, Jun 19, 2019 at 08:39:38PM CEST, sdf@fomichev.me wrote:
On 06/20, YueHaibing wrote:
quoted
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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
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.
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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
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
From: Stanislav Fomichev <sdf@fomichev.me> Date: 2019-06-21 00:33:21
On 06/20, Yuehaibing wrote:
On 2019/6/20 2:39, Stanislav Fomichev wrote:
quoted
On 06/20, YueHaibing wrote:
quoted
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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
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 :-)
Fri, Jun 21, 2019 at 02:33:17AM CEST, sdf@fomichev.me wrote:
On 06/20, Yuehaibing wrote:
quoted
On 2019/6/20 2:39, Stanislav Fomichev wrote:
quoted
On 06/20, YueHaibing wrote:
quoted
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 <redacted>
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)
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 :-)
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 <redacted>
---
drivers/net/bonding/bond_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
Acked-by: Jiri Pirko <redacted>
Could you please do the same for team? Thanks!
Wed, Jun 26, 2019 at 10:08:44AM CEST, yuehaibing@huawei.com wrote:
quoted
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 <redacted>
Acked-by: Jiri Pirko <redacted>
Could you please do the same for team? Thanks!
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.
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
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:
quoted
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.
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?
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2019-06-26 23:29:59
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:
quoted
On Wed, Jun 26, 2019 at 04:08:44PM +0800, YueHaibing wrote:
[...]
quoted
quoted
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.
[...]
quoted
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?
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 <redacted>
---
drivers/net/team/team.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: David Miller <davem@davemloft.net> Date: 2019-06-26 17:14:56
From: YueHaibing <redacted>
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 <redacted>
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 <redacted>
---
v2: remove redundant spaces
---
net/core/flow_dissector.c | 3 +++
1 file changed, 3 insertions(+)