[PATCH net-next 2/2] bonding: use l4 hash if available

[PATCH net-next 2/2] bonding: use l4 hash if available

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

If skb carries a l4 hash, no need to perform a flow dissection.

Performance is slightly better :

lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.39012e+06
lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.39393e+06
lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.39988e+06

After patch :

lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.43579e+06
lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.44304e+06
lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
2.44312e+06

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <tom@herbertland.com>
Cc: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/bonding/bond_main.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 771a449..9250d1e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3136,6 +3136,10 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 	struct flow_keys flow;
 	u32 hash;
 
+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
+	    skb->l4_hash)
+			return skb->hash;
+
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
 	    !bond_flow_dissect(bond, skb, &flow))
 		return bond_eth_hash(skb);

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Mahesh Bandewar]

On Tue, Sep 15, 2015 at 3:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> If skb carries a l4 hash, no need to perform a flow dissection.
>
> Performance is slightly better :
>
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39012e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39393e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39988e+06
>
> After patch :
>
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.43579e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.44304e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.44312e+06
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Cc: Mahesh Bandewar <maheshb@google.com>
> ---
>  drivers/net/bonding/bond_main.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 771a449..9250d1e 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3136,6 +3136,10 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>         struct flow_keys flow;
>         u32 hash;
>
> +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> +           skb->l4_hash)
if (ENCAP34 || LAYER34) && l4_hash) may be?

>
> +                       return skb->hash;
> +
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>             !bond_flow_dissect(bond, skb, &flow))
>                 return bond_eth_hash(skb);
>
>

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Eric Dumazet]

On Tue, 2015-09-15 at 15:54 -0700, Mahesh Bandewar wrote:

> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> > +           skb->l4_hash)
> if (ENCAP34 || LAYER34) && l4_hash) may be?

Hmm, traditional BOND_XMIT_POLICY_LAYER34 did not a full flow bisection
(tunnel awareness added in commit
32819dc1834866cb9547cb75f81af9edd58d33cd bonding: modify the old and add
new xmit hash policies)

This could radically change some setups and behavior.

BOND_XMIT_POLICY_ENCAP34 looks a better fit to me.

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Tom Herbert]

On Tue, Sep 15, 2015 at 3:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> If skb carries a l4 hash, no need to perform a flow dissection.
>
> Performance is slightly better :
>
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39012e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39393e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.39988e+06
>
> After patch :
>
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.43579e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.44304e+06
> lpaa5:~# ./super_netperf 200 -H lpaa6 -t TCP_RR -l 100
> 2.44312e+06
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> Cc: Mahesh Bandewar <maheshb@google.com>
> ---
>  drivers/net/bonding/bond_main.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 771a449..9250d1e 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3136,6 +3136,10 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>         struct flow_keys flow;
>         u32 hash;
>
> +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> +           skb->l4_hash)
> +                       return skb->hash;
> +
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>             !bond_flow_dissect(bond, skb, &flow))
>                 return bond_eth_hash(skb);
>
>
Ugh, bond_flow_dissect is yet another instance of customized flow
dissection! We should really clean this up. I suggest that in cases
were we want L4 hash a call to skb_get_hash should suffice. We can
create skb_get_l3hash when caller explicitly wants an L3 hash-- this
would return skb->hash if it's valid and skb->l4_hash is not set, else
call flow dissector with FLOW_DISSECTOR_F_STOP_AT_L3 and then do the
normal hash over flow keys (don't save result in skb->hash in this
case).

Tom

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Eric Dumazet]

On Tue, 2015-09-15 at 16:45 -0700, Tom Herbert wrote:
> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> > +           skb->l4_hash)
> > +                       return skb->hash;
> > +
> >         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
> >             !bond_flow_dissect(bond, skb, &flow))
> >                 return bond_eth_hash(skb);
> >
> >
> Ugh, bond_flow_dissect is yet another instance of customized flow
> dissection! We should really clean this up. I suggest that in cases
> were we want L4 hash a call to skb_get_hash should suffice. We can
> create skb_get_l3hash when caller explicitly wants an L3 hash-- this
> would return skb->hash if it's valid and skb->l4_hash is not set, else
> call flow dissector with FLOW_DISSECTOR_F_STOP_AT_L3 and then do the
> normal hash over flow keys (don't save result in skb->hash in this
> case).

This code predates all the change you did recently ;)

BTW, the simple xor weakness is showing up after
our change favoring even ports at connect() time, for a bonding device
with 2 or 4 slaves.

(commit 07f4c90062f8fc7c8c26f8f95324cbe8fa3145a5
"tcp/dccp: try to not exhaust ip_local_port_range in connect()")

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Mahesh Bandewar]

On Tue, Sep 15, 2015 at 4:20 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-09-15 at 15:54 -0700, Mahesh Bandewar wrote:
>
>> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
>> > +           skb->l4_hash)
>> if (ENCAP34 || LAYER34) && l4_hash) may be?
>
> Hmm, traditional BOND_XMIT_POLICY_LAYER34 did not a full flow bisection
> (tunnel awareness added in commit
> 32819dc1834866cb9547cb75f81af9edd58d33cd bonding: modify the old and add
> new xmit hash policies)
>
> This could radically change some setups and behavior.
>
> BOND_XMIT_POLICY_ENCAP34 looks a better fit to me.
>
Agreed, this will change flow distribution for LAYER34 policy but then
loose out on calculating hash per packet which I think is unnecessary.

This elimination of hash calculation is a good step but I'm feeling
that it's somehow tied to ENCAP policy which is actually orthogonal
and should be applied to LAYER34 also. However if that change in the
behavior for LAYER34 is considered too drastic then I'm perfectly fine
tying it to ENCAP34 policy.

>

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Tom Herbert]

On Tue, Sep 15, 2015 at 5:03 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-09-15 at 16:45 -0700, Tom Herbert wrote:
>> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
>> > +           skb->l4_hash)
>> > +                       return skb->hash;
>> > +
>> >         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>> >             !bond_flow_dissect(bond, skb, &flow))
>> >                 return bond_eth_hash(skb);
>> >
>> >
>> Ugh, bond_flow_dissect is yet another instance of customized flow
>> dissection! We should really clean this up. I suggest that in cases
>> were we want L4 hash a call to skb_get_hash should suffice. We can
>> create skb_get_l3hash when caller explicitly wants an L3 hash-- this
>> would return skb->hash if it's valid and skb->l4_hash is not set, else
>> call flow dissector with FLOW_DISSECTOR_F_STOP_AT_L3 and then do the
>> normal hash over flow keys (don't save result in skb->hash in this
>> case).
>
> This code predates all the change you did recently ;)
>
A more fundamental question is whether we can eliminate some of these
hashing types (I see five of them in if_bonding.h). Is there any
substantial difference between this and IPv4/v6 ECMP routing such that
they shouldn't all have the same path selection modes?

Tom

> BTW, the simple xor weakness is showing up after
> our change favoring even ports at connect() time, for a bonding device
> with 2 or 4 slaves.
>
> (commit 07f4c90062f8fc7c8c26f8f95324cbe8fa3145a5
> "tcp/dccp: try to not exhaust ip_local_port_range in connect()")
>
>
>
>

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Eric Dumazet]

On Tue, 2015-09-15 at 17:04 -0700, Mahesh Bandewar wrote:
> On Tue, Sep 15, 2015 at 4:20 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2015-09-15 at 15:54 -0700, Mahesh Bandewar wrote:
> >
> >> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> >> > +           skb->l4_hash)
> >> if (ENCAP34 || LAYER34) && l4_hash) may be?
> >
> > Hmm, traditional BOND_XMIT_POLICY_LAYER34 did not a full flow bisection
> > (tunnel awareness added in commit
> > 32819dc1834866cb9547cb75f81af9edd58d33cd bonding: modify the old and add
> > new xmit hash policies)
> >
> > This could radically change some setups and behavior.
> >
> > BOND_XMIT_POLICY_ENCAP34 looks a better fit to me.
> >
> Agreed, this will change flow distribution for LAYER34 policy but then
> loose out on calculating hash per packet which I think is unnecessary.

We added new bonding policy exactly for this.

If people are stuck with LAYER34, that is their choice.

> 
> This elimination of hash calculation is a good step but I'm feeling
> that it's somehow tied to ENCAP policy which is actually orthogonal
> and should be applied to LAYER34 also.

You can send a followup patch, once fully tested.

I've tested the ENCAP34 mode only, I do not want to add cycles for a
mode that is potentially a legacy one that nobody uses.

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Eric Dumazet]

On Tue, 2015-09-15 at 17:15 -0700, Tom Herbert wrote:

> A more fundamental question is whether we can eliminate some of these
> hashing types (I see five of them in if_bonding.h). Is there any
> substantial difference between this and IPv4/v6 ECMP routing such that
> they shouldn't all have the same path selection modes?

We had an issue on a router that did not like a change in the hashing
done by the host behind it.

Do not ask me for details that I cannot provide, but I would guess it is
better not changing legacy modes unilaterally.

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[Tom Herbert]

On Tue, Sep 15, 2015 at 5:03 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-09-15 at 16:45 -0700, Tom Herbert wrote:
>> > +       if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
>> > +           skb->l4_hash)
>> > +                       return skb->hash;
>> > +
>> >         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>> >             !bond_flow_dissect(bond, skb, &flow))
>> >                 return bond_eth_hash(skb);
>> >
>> >
>> Ugh, bond_flow_dissect is yet another instance of customized flow
>> dissection! We should really clean this up. I suggest that in cases
>> were we want L4 hash a call to skb_get_hash should suffice. We can
>> create skb_get_l3hash when caller explicitly wants an L3 hash-- this
>> would return skb->hash if it's valid and skb->l4_hash is not set, else
>> call flow dissector with FLOW_DISSECTOR_F_STOP_AT_L3 and then do the
>> normal hash over flow keys (don't save result in skb->hash in this
>> case).
>
> This code predates all the change you did recently ;)
>
> BTW, the simple xor weakness is showing up after
> our change favoring even ports at connect() time, for a bonding device
> with 2 or 4 slaves.
>
Right, xor as a packet hash should be eliminated. It seems possible
that all these modes can be implemented using flow_dissector and the
jhash. If I'm reading the meaning of modes correctly:

BOND_XMIT_POLICY_ENCAP34 is equivalent to skb_get_hash

BOND_XMIT_POLICY_LAYER23 is flow dissection with
FLOW_DISSECTOR_F_STOP_AT_L3 and then normal hash

BOND_XMIT_POLICY_LAYER34 is flow dissection with FLOW_DISSECTOR_F_STOP_AT_ENCAP

BOND_XMIT_POLICY_LAYER2 would be flow dissection with
FLOW_DISSECTOR_F_STOP_AT_L2 (new flag) and then normal hash

BOND_XMIT_POLICY_ENCAP23 is a little more interesting. This could be
accomplished with custom flow dissector targets that exclude L4
information (ports, flow label, key ID).

Also noticed a little bug in flow_dissector, we should get out on
FLOW_DISSECTOR_F_STOP_AT_L3 before IPv6 flow label is processed
(that's considered L4). I'll fix that.

Tom

> (commit 07f4c90062f8fc7c8c26f8f95324cbe8fa3145a5
> "tcp/dccp: try to not exhaust ip_local_port_range in connect()")
>
>
>
>

Re: [PATCH net-next 2/2] bonding: use l4 hash if available

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 15 Sep 2015 15:24:28 -0700

> +	if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> +	    skb->l4_hash)
> +			return skb->hash;

Applied, with the indentation of the return statement fixed up.
:-)