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 <[email protected]>
Cc: Veaceslav Falico <[email protected]>
Cc: Andy Gospodarek <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Thomas Davis <[email protected]>
Cc: Nikolay Aleksandrov <[email protected]>
Cc: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
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 20bbda1b36e1..32785e9d0295 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
On 21/05/2021 16:39, Nikolay Aleksandrov wrote:
> On 21/05/2021 16:27, 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 <[email protected]>
>> Cc: Veaceslav Falico <[email protected]>
>> Cc: Andy Gospodarek <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Jakub Kicinski <[email protected]>
>> Cc: Thomas Davis <[email protected]>
>> Cc: Nikolay Aleksandrov <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Jarod Wilson <[email protected]>
>> ---
>> Documentation/networking/bonding.rst | 13 +++++++++++++
>> drivers/net/bonding/bond_main.c | 18 ++++++++++++------
>> 2 files changed, 25 insertions(+), 6 deletions(-)
>>
>
> Hi,
> You seem to be missing netlink support for the new option. Code-wise the rest seems fine,
> my personal preference is still to make a configurable hash option and perhaps default to
> srcmac+vlan, i.e. it can be aliased with this hash option. I don't mind either way, but
> please add netlink support if it will be a new option as it's the preferred way for
> configuring.
>
> Thanks,
> Nik
>
Almost forgot - please include a cover letter with overview and motivation of the changes.
Also I guess these should be targeted at net-next (or at least the new option definitely should go
there), and please add changelog between patchset versions.
Cheers,
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 20bbda1b36e1..32785e9d0295 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 */
>>
>
On 21/05/2021 16:27, 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 <[email protected]>
> Cc: Veaceslav Falico <[email protected]>
> Cc: Andy Gospodarek <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Thomas Davis <[email protected]>
> Cc: Nikolay Aleksandrov <[email protected]>
> Cc: [email protected]
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> Documentation/networking/bonding.rst | 13 +++++++++++++
> drivers/net/bonding/bond_main.c | 18 ++++++++++++------
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
Hi,
You seem to be missing netlink support for the new option. Code-wise the rest seems fine,
my personal preference is still to make a configurable hash option and perhaps default to
srcmac+vlan, i.e. it can be aliased with this hash option. I don't mind either way, but
please add netlink support if it will be a new option as it's the preferred way for
configuring.
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 20bbda1b36e1..32785e9d0295 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 */
>
Nikolay Aleksandrov <[email protected]> wrote:
>On 21/05/2021 16:27, 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 <[email protected]>
>> Cc: Veaceslav Falico <[email protected]>
>> Cc: Andy Gospodarek <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Jakub Kicinski <[email protected]>
>> Cc: Thomas Davis <[email protected]>
>> Cc: Nikolay Aleksandrov <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Jarod Wilson <[email protected]>
>> ---
>> Documentation/networking/bonding.rst | 13 +++++++++++++
>> drivers/net/bonding/bond_main.c | 18 ++++++++++++------
>> 2 files changed, 25 insertions(+), 6 deletions(-)
>>
>
>Hi,
>You seem to be missing netlink support for the new option. Code-wise the rest seems fine,
>my personal preference is still to make a configurable hash option and perhaps default to
>srcmac+vlan, i.e. it can be aliased with this hash option. I don't mind either way, but
>please add netlink support if it will be a new option as it's the preferred way for
>configuring.
FWIW, I'm in agreement with Nik here; netlink support is
mandatory for any new options.
-J
>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 20bbda1b36e1..32785e9d0295 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 */
---
-Jay Vosburgh, [email protected]