From: Jarod Wilson <jarod@redhat.com>
To: Jay Vosburgh <jay.vosburgh@canonical.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
linux-kernel@vger.kernel.org,
Veaceslav Falico <vfalico@gmail.com>,
Andy Gospodarek <andy@greyhouse.net>,
"David S. Miller" <davem@davemloft.net>,
Thomas Davis <tadavis@lbl.gov>,
netdev@vger.kernel.org
Subject: Re: [PATCH net-next v2] bonding: add a vlan+mac tx hashing option
Date: Fri, 15 Jan 2021 10:08:57 -0500 [thread overview]
Message-ID: <20210115150857.GA1176575@redhat.com> (raw)
In-Reply-To: <8507.1610661271@famine>
On Thu, Jan 14, 2021 at 01:54:31PM -0800, Jay Vosburgh wrote:
> Jarod Wilson <jarod@redhat.com> wrote:
>
> >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:
> >> > 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.
> >>
> >> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> >> > ---
> >> > 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
>
> I notice that the code calls it "VLAN_SRCMAC" but the
> user-facing nomenclature is "vlan+mac"; I tend to lean towards having
> the user visible name also be "vlan+srcmac". Both for consistency, and
> just in case someone someday wants "vlan+dstmac". And you did ask for
> preference on this in a separate email.
That's valid. I was trying to keep it short, but it does muddy the waters
a bit by not including src. I'll adjust accordingly and resend the
userspace bit too.
...
> Yah, the existing L2 hash is pretty weak. It might be possible
> to squeeze this into the existing bond_xmit_hash a bit better, if the
> hash is two u32s. The first being the first 32 bits of the MAC, and the
> second being the last 16 bits of the MAC combined with the 16 bit VLAN
> tag.
>
> There's already logic at the end of bond_xmit_hash to reduce a
> u32 into the final hash that perhaps could be leveraged.
>
> Thinking about it, though, all the ways to combine that data
> together end up being pretty vile ("*(u32 *)ðhdr->h_source[0]" sorts
> of things).
Yeah, I'd worry that bond_xmit_hash() is already getting a bit complicated
to follow and understand, and that would make it even more so.
> >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];
>
> I think this will shift h_source[0] and [1] into oblivion.
Argh, yep, 48 bits don't fit into a u32. Okay, so I'll replace that with a
u32 srcmac_vendor and u32 srcmac_dev, but they'll only have 24 bits of data
in them, then return vlan ^ srcmac_vendor ^ srcmac_dev, I think.
--
Jarod Wilson
jarod@redhat.com
next prev parent reply other threads:[~2021-01-15 15:10 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-18 19:30 [RFC PATCH net-next] bonding: add a vlan+srcmac tx hashing option Jarod Wilson
2020-12-19 0:18 ` Jay Vosburgh
2021-01-08 0:03 ` Jarod Wilson
2021-01-12 21:12 ` Jarod Wilson
2021-01-12 21:39 ` Jay Vosburgh
2021-01-12 22:32 ` Jarod Wilson
2020-12-28 10:11 ` Jiri Pirko
2021-01-07 23:58 ` Jarod Wilson
2021-01-08 13:12 ` Jiri Pirko
2021-01-08 15:21 ` Jarod Wilson
2021-01-15 2:02 ` question about bonding mode 4 moyufeng
2021-01-23 6:10 ` moyufeng
2021-01-29 19:11 ` Jay Vosburgh
2021-01-30 9:41 ` moyufeng
2021-01-13 22:35 ` [PATCH net-next v2] bonding: add a vlan+mac tx hashing option Jarod Wilson
2021-01-13 23:41 ` [PATCH iproute2] bond: support xmit_hash_policy=vlan+mac Jarod Wilson
2021-01-15 15:12 ` Jarod Wilson
2021-01-15 19:21 ` [PATCH iproute2 v2] bond: support xmit_hash_policy=vlan+srcmac Jarod Wilson
2021-01-23 18:35 ` David Ahern
2021-01-14 1:58 ` [PATCH net-next v2] bonding: add a vlan+mac tx hashing option Jakub Kicinski
2021-01-14 21:11 ` Jarod Wilson
2021-01-14 21:23 ` Jakub Kicinski
2021-01-14 21:42 ` Jarod Wilson
2021-01-14 21:54 ` Jay Vosburgh
2021-01-15 15:08 ` Jarod Wilson [this message]
2021-01-15 19:21 ` [PATCH net-next v3] bonding: add a vlan+srcmac " Jarod Wilson
2021-01-18 23:10 ` David Ahern
2021-01-19 1:04 ` Jarod Wilson
2021-01-19 1:09 ` [PATCH net-next v4] " Jarod Wilson
2021-01-20 6:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210115150857.GA1176575@redhat.com \
--to=jarod@redhat.com \
--cc=andy@greyhouse.net \
--cc=davem@davemloft.net \
--cc=jay.vosburgh@canonical.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=tadavis@lbl.gov \
--cc=vfalico@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).