[net] bonding: discard lowest hash bit for 802.3ad layer3+4

After commit 07f4c90062f8 ("tcp/dccp: try to not exhaust ip_local_port_range
in connect()"), we will try to use even ports for connect(). Then If an
application (seen clearly with iperf) opens multiple streams to the same
destination IP and port, each stream will be given an even source port.

So the bonding driver's simple xmit_hash_policy based on layer3+4 addressing
will always hash all these streams to the same interface. And the total
throughput will limited to a single slave.

Change the tcp code will impact the whole tcp behavior, only for bonding
usage. Paolo Abeni suggested fix this by changing the bonding code only,
which should be more reasonable, and less impact.

Fix this by discarding the lowest hash bit because it contains little entropy.
After the fix we can re-balance between slaves.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Sun, Nov 5, 2017 at 6:56 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> After commit 07f4c90062f8 ("tcp/dccp: try to not exhaust ip_local_port_range
> in connect()"), we will try to use even ports for connect(). Then If an
> application (seen clearly with iperf) opens multiple streams to the same
> destination IP and port, each stream will be given an even source port.
>
> So the bonding driver's simple xmit_hash_policy based on layer3+4 addressing
> will always hash all these streams to the same interface. And the total
> throughput will limited to a single slave.
>
> Change the tcp code will impact the whole tcp behavior, only for bonding
> usage. Paolo Abeni suggested fix this by changing the bonding code only,
> which should be more reasonable, and less impact.
>
> Fix this by discarding the lowest hash bit because it contains little entropy.
> After the fix we can re-balance between slaves.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c99dc59..728fa08 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3237,7 +3237,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
>             skb->l4_hash)
> -               return skb->hash;
> +               return skb->hash >> 1;

Why are you changing this part ?

The l4 hash provided by local TCP stack does not use a pathological
XOR based on ports/addresses,
but a random value with pretty good entropy.

No need to try do 'enhance' it by actually being slightly worse.


>
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>             !bond_flow_dissect(bond, skb, &flow))
> @@ -3253,7 +3253,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>         hash ^= (hash >> 16);
>         hash ^= (hash >> 8);
>
> -       return hash;
> +       return hash >> 1;
>  }
>
>  /*-------------------------- Device entry points ----------------------------*/
> --
> 2.5.5
>

On Sun, Nov 05, 2017 at 01:38:47PM -0800, Eric Dumazet wrote:
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index c99dc59..728fa08 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -3237,7 +3237,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> >
> >         if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> >             skb->l4_hash)
> > -               return skb->hash;
> > +               return skb->hash >> 1;
> 
> Why are you changing this part ?
> 
> The l4 hash provided by local TCP stack does not use a pathological
> XOR based on ports/addresses,
> but a random value with pretty good entropy.
> 

Oh, my bad. I only tested the patch works, but not carefully check the
skb->hash when skb->l4_hash is 1. Will send a v2 patch to fix this.

Regards
Hangbin