On Wed, Jan 13, 2021 at 05:58:18PM -0800, Jakub Kicinski wrote:

On Wed, 13 Jan 2021 17:35:48 -0500 Jarod Wilson wrote:

quoted

This comes from an end-user request, where they're running multiple VMs on
hosts with bonded interfaces connected to some interest switch topologies,
where 802.3ad isn't an option. They're currently running a proprietary
solution that effectively achieves load-balancing of VMs and bandwidth
utilization improvements with a similar form of transmission algorithm.

Basically, each VM has it's own vlan, so it always sends its traffic out
the same interface, unless that interface fails. Traffic gets split
between the interfaces, maintaining a consistent path, with failover still
available if an interface goes down.

This has been rudimetarily tested to provide similar results, suitable for
them to use to move off their current proprietary solution. A patch for
iproute2 is forthcoming as well, to properly support the new mode there as
well.

quoted

Signed-off-by: Jarod Wilson <redacted>
---
v2: verified netlink interfaces working, added Documentation, changed
tx hash mode name to vlan+mac for consistency and clarity.

 Documentation/networking/bonding.rst | 13 +++++++++++++
 drivers/net/bonding/bond_main.c      | 27 +++++++++++++++++++++++++--
 drivers/net/bonding/bond_options.c   |  1 +
 include/linux/netdevice.h            |  1 +
 include/uapi/linux/if_bonding.h      |  1 +
 5 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index adc314639085..c78ceb7630a0 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst

@@ -951,6 +951,19 @@ xmit_hash_policy
 		packets will be distributed according to the encapsulated
 		flows.
 
+	vlan+mac
+
+		This policy uses a very rudimentary vland ID and source mac
+		ID hash to load-balance traffic per-vlan, with failover
+		should one leg fail. The intended use case is for a bond
+		shared by multiple virtual machines, all configured to
+		use their own vlan, to give lacp-like functionality
+		without requiring lacp-capable switching hardware.
+
+		The formula for the hash is simply
+
+		hash = (vlan ID) XOR (source MAC)

But in the code it's only using one byte of the MAC, currently.

I think that's fine for the particular use case but should we call out
explicitly in the commit message why it's considered sufficient?

Someone can change it later, if needed, but best if we spell out the
current motivation.

In truth, this code started out as a copy of bond_eth_hash(), which also
only uses the last byte, though of both source and destination macs. In
the typical use case for the requesting user, the bond is formed from two
onboard NICs, which typically have adjacent mac addresses, i.e.,
AA:BB:CC:DD:EE:01 and AA:BB:CC:DD:EE:02, so only the last byte is really
relevant to hash differently, but in thinking about it, a replacement NIC
because an onboard one died could have the same last byte, and maybe we
ought to just go full source mac right off the go here.

Something like this instead maybe:

static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
{
        struct ethhdr *mac_hdr = (struct ethhdr *)skb_mac_header(skb);
        u32 srcmac = 0;
        u16 vlan;
        int i;

        for (i = 0; i < ETH_ALEN; i++)
                srcmac = (srcmac << 8) | mac_hdr->h_source[i];

        if (!skb_vlan_tag_present(skb))
                return srcmac;

        vlan = skb_vlan_tag_get(skb);

        return vlan ^ srcmac;
}

Then the documentation is spot-on, and we're future-proof, though
marginally less performant in calculating the hash, which may have been a
consideration when the original function was written, but is probably
basically irrelevant w/modern systems...

quoted

 	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

quoted

+static inline u32 bond_vlan_srcmac_hash(struct sk_buff *skb)

Can we drop the inline? It's a static function called once.

Works for me. That was also inherited by copying bond_eth_hash(). :)

quoted

+{
+	struct ethhdr *mac_hdr = (struct ethhdr *)skb_mac_header(skb);

I don't see anything in the patch making sure the interface actually
has a L2 header. Should we validate somehow the ifc is Ethernet?

I don't think it's necessary. There doesn't appear to be any explicit
check for BOND_XMIT_POLICY_LAYER2 either. I believe we're guaranteed to
not have anything but an ethernet header here, as the only other type I'm
aware of being supported is Infiniband, but we limit that to active-backup
only, and xmit_hash_policy isn't valid for active-backup.

-- 
Jarod Wilson
jarod@redhat.com