[PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Yevgeny Petrilin]

Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
 drivers/net/mlx4/en_rx.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/mlx4/en_rx.c b/drivers/net/mlx4/en_rx.c
index 62dd21b..bb4d66a 100644
--- a/drivers/net/mlx4/en_rx.c
+++ b/drivers/net/mlx4/en_rx.c
@@ -610,6 +610,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 					gro_skb->data_len = length;
 					gro_skb->truesize += length;
 					gro_skb->ip_summed = CHECKSUM_UNNECESSARY;
+					gro_skb->rxhash = be32_to_cpu(cqe->immed_rss_invalid) << 24;
+					skb_record_rx_queue(gro_skb, cq->ring);
 
 					if (priv->vlgrp && (cqe->vlan_my_qpn &
 							    cpu_to_be32(MLX4_CQE_VLAN_PRESENT_MASK)))
@@ -643,6 +645,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 			goto next;
 		}
 
+		skb->rxhash = be32_to_cpu(cqe->immed_rss_invalid) << 24;
 		skb->ip_summed = ip_summed;
 		skb->protocol = eth_type_trans(skb, dev);
 		skb_record_rx_queue(skb, cq->ring);
-- 
1.6.0.2

Re: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Ben Hutchings]

On Wed, 2011-05-04 at 16:37 +0300, Yevgeny Petrilin wrote:
> Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
> ---
>  drivers/net/mlx4/en_rx.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/mlx4/en_rx.c b/drivers/net/mlx4/en_rx.c
> index 62dd21b..bb4d66a 100644
> --- a/drivers/net/mlx4/en_rx.c
> +++ b/drivers/net/mlx4/en_rx.c
> @@ -610,6 +610,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>  					gro_skb->data_len = length;
>  					gro_skb->truesize += length;
>  					gro_skb->ip_summed = CHECKSUM_UNNECESSARY;
> +					gro_skb->rxhash = be32_to_cpu(cqe->immed_rss_invalid) << 24;
> +					skb_record_rx_queue(gro_skb, cq->ring);

An 8-bit hash is almost useless.  It's entirely useless if you then
shift it into the top bits of rxhash.

Ben.

>  					if (priv->vlgrp && (cqe->vlan_my_qpn &
>  							    cpu_to_be32(MLX4_CQE_VLAN_PRESENT_MASK)))
> @@ -643,6 +645,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
>  			goto next;
>  		}
>  
> +		skb->rxhash = be32_to_cpu(cqe->immed_rss_invalid) << 24;
>  		skb->ip_summed = ip_summed;
>  		skb->protocol = eth_type_trans(skb, dev);
>  		skb_record_rx_queue(skb, cq->ring);

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Eric Dumazet]

Le mercredi 04 mai 2011 à 15:44 +0100, Ben Hutchings a écrit :
> On Wed, 2011-05-04 at 16:37 +0300, Yevgeny Petrilin wrote:
> > Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
> > ---
> >  drivers/net/mlx4/en_rx.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/mlx4/en_rx.c b/drivers/net/mlx4/en_rx.c
> > index 62dd21b..bb4d66a 100644
> > --- a/drivers/net/mlx4/en_rx.c
> > +++ b/drivers/net/mlx4/en_rx.c
> > @@ -610,6 +610,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
> >  					gro_skb->data_len = length;
> >  					gro_skb->truesize += length;
> >  					gro_skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +					gro_skb->rxhash = be32_to_cpu(cqe->immed_rss_invalid) << 24;
> > +					skb_record_rx_queue(gro_skb, cq->ring);
> 
> An 8-bit hash is almost useless.  It's entirely useless if you then
> shift it into the top bits of rxhash.
> 

Agreed. This is very bad.

Yevgeny probably did this shift because get_rps_cpu()
does :

tcpu = map->cpus[((u64) skb->rxhash * map->len) >> 32];

(If rxhash is not a pure random u32 distribution, then high order bits
are more important than low order bits)

RE: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Yevgeny Petrilin]

> >
> > An 8-bit hash is almost useless.  It's entirely useless if you then
> > shift it into the top bits of rxhash.
> >
> 
> Agreed. This is very bad.
> 
> Yevgeny probably did this shift because get_rps_cpu() does :
> 
> tcpu = map->cpus[((u64) skb->rxhash * map->len) >> 32];
> 
> (If rxhash is not a pure random u32 distribution, then high order bits are more important than low order bits)
> 
> 
Eric, you are correct.
We do plan to enable full 32 bit hash for our devices.
Once it is done, we will naturally use the whole 32 bits.
In the meanwhile, even with this change we see improved performance when enabling RPS.

Thanks,
Yevgeny

RE: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Eric Dumazet]

Le jeudi 05 mai 2011 à 06:47 +0000, Yevgeny Petrilin a écrit :
> > 
> Eric, you are correct.
> We do plan to enable full 32 bit hash for our devices.
> Once it is done, we will naturally use the whole 32 bits.
> In the meanwhile, even with this change we see improved performance when enabling RPS.

Hmm, thats strange because you have only 256 possible values for rxhash,
and it's OK for maybe one hundred flows. (I am talking not of RPS but
RFS here)

So your patch is a win only for small hosts, or particular workloads
(did I say biased benchmarks ? ;) )

Really, we better use the linux/software full 32bit rxhash in this case,
and wait for your 32bit full support.

RE: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Eric Dumazet]

Le jeudi 05 mai 2011 à 08:55 +0200, Eric Dumazet a écrit :
> Le jeudi 05 mai 2011 à 06:47 +0000, Yevgeny Petrilin a écrit :
> > > 
> > Eric, you are correct.
> > We do plan to enable full 32 bit hash for our devices.
> > Once it is done, we will naturally use the whole 32 bits.
> > In the meanwhile, even with this change we see improved performance when enabling RPS.
> 
> Hmm, thats strange because you have only 256 possible values for rxhash,
> and it's OK for maybe one hundred flows. (I am talking not of RPS but
> RFS here)
> 
> So your patch is a win only for small hosts, or particular workloads
> (did I say biased benchmarks ? ;) )
> 
> Really, we better use the linux/software full 32bit rxhash in this case,
> and wait for your 32bit full support.
> 
> 

BTW, before you submit another rxhash patch, please make sure ethtool
support is included. An admin must be able to switch off hardware
support :

ethtool -K eth0 rxhash off

RE: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[Yevgeny Petrilin]

> > > >
> > > Eric, you are correct.
> > > We do plan to enable full 32 bit hash for our devices.
> > > Once it is done, we will naturally use the whole 32 bits.
> > > In the meanwhile, even with this change we see improved performance when enabling RPS.
> >
> > Hmm, thats strange because you have only 256 possible values for rxhash,
> > and it's OK for maybe one hundred flows. (I am talking not of RPS but
> > RFS here)
> >
> > So your patch is a win only for small hosts, or particular workloads
> > (did I say biased benchmarks ? ;) )
> >
> > Really, we better use the linux/software full 32bit rxhash in this case,
> > and wait for your 32bit full support.
> >
> >
> 
> BTW, before you submit another rxhash patch, please make sure ethtool
> support is included. An admin must be able to switch off hardware
> support :
> 
> ethtool -K eth0 rxhash off
> 
> 
Thanks for this input

Re: [PATCH] mlx4_en: Setting RSS hash result to skb->rxhash field

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 05 May 2011 08:55:18 +0200

> Really, we better use the linux/software full 32bit rxhash in this case,
> and wait for your 32bit full support.

Agreed.