linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 0/2] bonding/balance-alb: support VMs behind bridges better
@ 2021-05-27 20:00 Jarod Wilson
  2021-05-27 20:00 ` [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option Jarod Wilson
  2021-05-27 20:00 ` [PATCH net-next v3 2/2] bonding/balance-alb: put all slaves into promisc Jarod Wilson
  0 siblings, 2 replies; 4+ messages in thread
From: Jarod Wilson @ 2021-05-27 20:00 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jarod Wilson

I've been further educated on a use case, where a bridge sits on top of
a bond, with multiple vnetX interfaces attached to virtual machines,
also acting as ports of the bridge. Each leg of the bond goes to a
different switch, but there is NO mlag/vpc in play, the bonding driver
has to handle traffic that loops back appropriately to avoid breaking
transmission. Rather than adding some sort of mac filtering to
balance-xor mode, we switched to using balance-alb, which already does
some of this, and with the tweaks provided in this series, empirically
seems to behave as desired in actual operation.

v2 attempts to support srcmac-only hashing via a modparam instead of by
adding yet another hashing mode, as well as cleaning up and clarifying
commit messages.

v3 omits two patches from the series that prove unnecessary when all
slaves get promiscuity propagated to them.

Jarod Wilson (2):
  bonding: add pure source-mac-based tx hashing option
  bonding/balance-alb: put all slaves into promisc

 Documentation/networking/bonding.rst | 13 +++++++++++++
 drivers/net/bonding/bond_main.c      | 23 +++++++++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

-- 
2.30.2


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

* [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option
  2021-05-27 20:00 [PATCH net-next v3 0/2] bonding/balance-alb: support VMs behind bridges better Jarod Wilson
@ 2021-05-27 20:00 ` Jarod Wilson
  2021-05-28 10:10   ` Nikolay Aleksandrov
  2021-05-27 20:00 ` [PATCH net-next v3 2/2] bonding/balance-alb: put all slaves into promisc Jarod Wilson
  1 sibling, 1 reply; 4+ messages in thread
From: Jarod Wilson @ 2021-05-27 20:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jarod Wilson, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
	David S. Miller, Jakub Kicinski, Thomas Davis,
	Nikolay Aleksandrov, netdev

As it turns out, a pure source-mac only tx hash has a place for some VM
setups. The previously added vlan+srcmac hash doesn't work as well for a
VM with a single MAC and multiple vlans -- these types of setups path
traffic more efficiently if the load is split by source mac alone. Enable
this by way of an extra module parameter, rather than adding yet another
hashing method in the tx fast path.

Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Thomas Davis <tadavis@lbl.gov>
Cc: Nikolay Aleksandrov <nikolay@nvidia.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 Documentation/networking/bonding.rst | 13 +++++++++++++
 drivers/net/bonding/bond_main.c      | 18 ++++++++++++------
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index 62f2aab8eaec..767dbb49018b 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -707,6 +707,13 @@ mode
 		swapped with the new curr_active_slave that was
 		chosen.
 
+novlan_srcmac
+
+	When using the vlan+srcmac xmit_hash_policy, there may be cases where
+	omitting the vlan from the hash is beneficial. This can be done with
+	an extra module parameter here. The default value is 0 to include
+	vlan ID in the transmit hash.
+
 num_grat_arp,
 num_unsol_na
 
@@ -964,6 +971,12 @@ xmit_hash_policy
 
 		hash = (vlan ID) XOR (source MAC vendor) XOR (source MAC dev)
 
+		Optionally, if the module parameter novlan_srcmac=1 is set,
+		the vlan ID is omitted from the hash and only the source MAC
+		address is used, reducing the hash to
+
+		hash = (source MAC vendor) XOR (source MAC dev)
+
 	The default value is layer2.  This option was added in bonding
 	version 2.6.3.  In earlier versions of bonding, this parameter
 	does not exist, and the layer2 policy is the only policy.  The
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7e469c203ca5..666051f91d70 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -107,6 +107,7 @@ static char *lacp_rate;
 static int min_links;
 static char *ad_select;
 static char *xmit_hash_policy;
+static int novlan_srcmac;
 static int arp_interval;
 static char *arp_ip_target[BOND_MAX_ARP_TARGETS];
 static char *arp_validate;
@@ -168,6 +169,9 @@ MODULE_PARM_DESC(xmit_hash_policy, "balance-alb, balance-tlb, balance-xor, 802.3
 				   "0 for layer 2 (default), 1 for layer 3+4, "
 				   "2 for layer 2+3, 3 for encap layer 2+3, "
 				   "4 for encap layer 3+4, 5 for vlan+srcmac");
+module_param(novlan_srcmac, int, 0);
+MODULE_PARM_DESC(novlan_srcmac, "include vlan ID in vlan+srcmac xmit_hash_policy or not; "
+			      "0 to include it (default), 1 to exclude it");
 module_param(arp_interval, int, 0);
 MODULE_PARM_DESC(arp_interval, "arp interval in milliseconds");
 module_param_array(arp_ip_target, charp, NULL, 0);
@@ -3523,9 +3527,9 @@ static bool bond_flow_ip(struct sk_buff *skb, struct flow_keys *fk,
 
 static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
 {
-	struct ethhdr *mac_hdr = (struct ethhdr *)skb_mac_header(skb);
+	struct ethhdr *mac_hdr = eth_hdr(skb);
 	u32 srcmac_vendor = 0, srcmac_dev = 0;
-	u16 vlan;
+	u32 hash;
 	int i;
 
 	for (i = 0; i < 3; i++)
@@ -3534,12 +3538,14 @@ static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
 	for (i = 3; i < ETH_ALEN; i++)
 		srcmac_dev = (srcmac_dev << 8) | mac_hdr->h_source[i];
 
-	if (!skb_vlan_tag_present(skb))
-		return srcmac_vendor ^ srcmac_dev;
+	hash = srcmac_vendor ^ srcmac_dev;
+
+	if (novlan_srcmac || !skb_vlan_tag_present(skb))
+		return hash;
 
-	vlan = skb_vlan_tag_get(skb);
+	hash ^= skb_vlan_tag_get(skb);
 
-	return vlan ^ srcmac_vendor ^ srcmac_dev;
+	return hash;
 }
 
 /* Extract the appropriate headers based on bond's xmit policy */
-- 
2.30.2


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

* [PATCH net-next v3 2/2] bonding/balance-alb: put all slaves into promisc
  2021-05-27 20:00 [PATCH net-next v3 0/2] bonding/balance-alb: support VMs behind bridges better Jarod Wilson
  2021-05-27 20:00 ` [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option Jarod Wilson
@ 2021-05-27 20:00 ` Jarod Wilson
  1 sibling, 0 replies; 4+ messages in thread
From: Jarod Wilson @ 2021-05-27 20:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jarod Wilson, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
	David S. Miller, Jakub Kicinski, Thomas Davis, netdev

Unlike most other modes with a primary interface, ALB mode bonding can
receive on all slaves. That includes traffic destined for a non-local MAC
behind a bridge on top of the bond. In such setups, the bridge driver puts
all ports (including the bond) into promiscuous mode, but we're not
propagating that change down to the non-primary slaves in ALB mode. As a
result, incoming traffic to those interfaces gets dropped. Since ALB can
receive on all slaves, all slaves should get promiscuity changes
propagated to them.

Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Thomas Davis <tadavis@lbl.gov>
Cc: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/bonding/bond_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 666051f91d70..3933219ae773 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -647,9 +647,10 @@ static int bond_check_dev_link(struct bonding *bond,
 static int bond_set_promiscuity(struct bonding *bond, int inc)
 {
 	struct list_head *iter;
-	int err = 0;
+	int mode, err = 0;
 
-	if (bond_uses_primary(bond)) {
+	mode = BOND_MODE(bond);
+	if (mode == BOND_MODE_ACTIVEBACKUP || mode == BOND_MODE_TLB) {
 		struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
 
 		if (curr_active)
-- 
2.30.2


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

* Re: [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option
  2021-05-27 20:00 ` [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option Jarod Wilson
@ 2021-05-28 10:10   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 4+ messages in thread
From: Nikolay Aleksandrov @ 2021-05-28 10:10 UTC (permalink / raw)
  To: Jarod Wilson, linux-kernel
  Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller,
	Jakub Kicinski, Thomas Davis, netdev

On 27/05/2021 23:00, Jarod Wilson wrote:
> As it turns out, a pure source-mac only tx hash has a place for some VM
> setups. The previously added vlan+srcmac hash doesn't work as well for a
> VM with a single MAC and multiple vlans -- these types of setups path
> traffic more efficiently if the load is split by source mac alone. Enable
> this by way of an extra module parameter, rather than adding yet another
> hashing method in the tx fast path.
> 
> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> Cc: Veaceslav Falico <vfalico@gmail.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Thomas Davis <tadavis@lbl.gov>
> Cc: Nikolay Aleksandrov <nikolay@nvidia.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  Documentation/networking/bonding.rst | 13 +++++++++++++
>  drivers/net/bonding/bond_main.c      | 18 ++++++++++++------
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 

Hi,
Is there any difference between v2 and v3 ?
Me and Jay commented about netlink support for the new option, any new option should be
configurable via netlink. Please include a changelog between versions so it's easier for
reviewers.

Thanks,
 Nik

> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index 62f2aab8eaec..767dbb49018b 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -707,6 +707,13 @@ mode
>  		swapped with the new curr_active_slave that was
>  		chosen.
>  
> +novlan_srcmac
> +
> +	When using the vlan+srcmac xmit_hash_policy, there may be cases where
> +	omitting the vlan from the hash is beneficial. This can be done with
> +	an extra module parameter here. The default value is 0 to include
> +	vlan ID in the transmit hash.
> +
>  num_grat_arp,
>  num_unsol_na
>  
> @@ -964,6 +971,12 @@ xmit_hash_policy
>  
>  		hash = (vlan ID) XOR (source MAC vendor) XOR (source MAC dev)
>  
> +		Optionally, if the module parameter novlan_srcmac=1 is set,
> +		the vlan ID is omitted from the hash and only the source MAC
> +		address is used, reducing the hash to
> +
> +		hash = (source MAC vendor) XOR (source MAC dev)
> +
>  	The default value is layer2.  This option was added in bonding
>  	version 2.6.3.  In earlier versions of bonding, this parameter
>  	does not exist, and the layer2 policy is the only policy.  The
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7e469c203ca5..666051f91d70 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -107,6 +107,7 @@ static char *lacp_rate;
>  static int min_links;
>  static char *ad_select;
>  static char *xmit_hash_policy;
> +static int novlan_srcmac;
>  static int arp_interval;
>  static char *arp_ip_target[BOND_MAX_ARP_TARGETS];
>  static char *arp_validate;
> @@ -168,6 +169,9 @@ MODULE_PARM_DESC(xmit_hash_policy, "balance-alb, balance-tlb, balance-xor, 802.3
>  				   "0 for layer 2 (default), 1 for layer 3+4, "
>  				   "2 for layer 2+3, 3 for encap layer 2+3, "
>  				   "4 for encap layer 3+4, 5 for vlan+srcmac");
> +module_param(novlan_srcmac, int, 0);
> +MODULE_PARM_DESC(novlan_srcmac, "include vlan ID in vlan+srcmac xmit_hash_policy or not; "
> +			      "0 to include it (default), 1 to exclude it");
>  module_param(arp_interval, int, 0);
>  MODULE_PARM_DESC(arp_interval, "arp interval in milliseconds");
>  module_param_array(arp_ip_target, charp, NULL, 0);
> @@ -3523,9 +3527,9 @@ static bool bond_flow_ip(struct sk_buff *skb, struct flow_keys *fk,
>  
>  static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
>  {
> -	struct ethhdr *mac_hdr = (struct ethhdr *)skb_mac_header(skb);
> +	struct ethhdr *mac_hdr = eth_hdr(skb);
>  	u32 srcmac_vendor = 0, srcmac_dev = 0;
> -	u16 vlan;
> +	u32 hash;
>  	int i;
>  
>  	for (i = 0; i < 3; i++)
> @@ -3534,12 +3538,14 @@ static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
>  	for (i = 3; i < ETH_ALEN; i++)
>  		srcmac_dev = (srcmac_dev << 8) | mac_hdr->h_source[i];
>  
> -	if (!skb_vlan_tag_present(skb))
> -		return srcmac_vendor ^ srcmac_dev;
> +	hash = srcmac_vendor ^ srcmac_dev;
> +
> +	if (novlan_srcmac || !skb_vlan_tag_present(skb))
> +		return hash;
>  
> -	vlan = skb_vlan_tag_get(skb);
> +	hash ^= skb_vlan_tag_get(skb);
>  
> -	return vlan ^ srcmac_vendor ^ srcmac_dev;
> +	return hash;
>  }
>  
>  /* Extract the appropriate headers based on bond's xmit policy */
> 


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

end of thread, other threads:[~2021-05-28 10:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-27 20:00 [PATCH net-next v3 0/2] bonding/balance-alb: support VMs behind bridges better Jarod Wilson
2021-05-27 20:00 ` [PATCH net-next v3 1/2] bonding: add pure source-mac-based tx hashing option Jarod Wilson
2021-05-28 10:10   ` Nikolay Aleksandrov
2021-05-27 20:00 ` [PATCH net-next v3 2/2] bonding/balance-alb: put all slaves into promisc Jarod Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).