[RFC] be2net: add rxhash support

[RFC] be2net: add rxhash support

[Eric Dumazet]

Ajit, it seems be2net provides RSS hash value in rx compl descriptor ?

Could we feed skb->rxhash with it ?

Thanks !

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 0bdccb1..f2db5b2 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -1038,6 +1038,9 @@ static void be_rx_compl_process(struct be_adapter *adapter,
 
 	skb->truesize = skb->len + sizeof(struct sk_buff);
 	skb->protocol = eth_type_trans(skb, adapter->netdev);
+	if (adapter->netdev->features & NETIF_F_RXHASH)
+		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl, rsshash, rxcp);
+
 
 	vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp);
 	vtm = AMAP_GET_BITS(struct amap_eth_rx_compl, vtm, rxcp);
@@ -1099,6 +1102,9 @@ static void be_rx_compl_process_gro(struct be_adapter *adapter,
 		return;
 	}
 
+	if (adapter->netdev->features & NETIF_F_RXHASH)
+		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl, rsshash, rxcp);
+
 	remaining = pkt_size;
 	for (i = 0, j = -1; i < num_rcvd; i++) {
 		page_info = get_rx_page_info(adapter, rxo, rxq_idx);
@@ -2618,6 +2624,7 @@ static void be_netdev_init(struct net_device *netdev)
 		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
 		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_GRO | NETIF_F_TSO6;
+	netdev->features |= NETIF_F_RXHASH;
 
 	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
 		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;

Re: [RFC] be2net: add rxhash support

[Ajit Khaparde]

> -----Original Message-----
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Sent: Thursday, February 24, 2011 2:25 PM
> To: Khaparde, Ajit
> Cc: netdev@vger.kernel.org
> Subject: [RFC] be2net: add rxhash support
> 
> Ajit, it seems be2net provides RSS hash value in rx compl descriptor ?
> 
> Could we feed skb->rxhash with it ?
> 
> Thanks !
Thanks Eric. Sure.
This is a long pending change which fell through the cracks.
But then because hashing is enabled in the device only when
Number of Rx Queues is > 1, I would suggest the following patch.

Unaware of exact conventions, I have added signed-off-by to the patch already.

Thanks

-----
[PATCH net-next] be2net: add rxhash support

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@emulex.com>
---
 drivers/net/benet/be_main.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 26f9c56..8c4b782 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -1038,6 +1038,10 @@ static void be_rx_compl_process(struct be_adapter *adapter,
 
 	skb->truesize = skb->len + sizeof(struct sk_buff);
 	skb->protocol = eth_type_trans(skb, adapter->netdev);
+	if (adapter->netdev->features & NETIF_F_RXHASH)
+		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
+					rsshash, rxcp);
+
 
 	vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp);
 	vtm = AMAP_GET_BITS(struct amap_eth_rx_compl, vtm, rxcp);
@@ -1099,6 +1103,10 @@ static void be_rx_compl_process_gro(struct be_adapter *adapter,
 		return;
 	}
 
+	if (adapter->netdev->features & NETIF_F_RXHASH)
+		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
+						rsshash, rxcp);
+
 	remaining = pkt_size;
 	for (i = 0, j = -1; i < num_rcvd; i++) {
 		page_info = get_rx_page_info(adapter, rxo, rxq_idx);
@@ -2619,6 +2627,9 @@ static void be_netdev_init(struct net_device *netdev)
 		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_GRO | NETIF_F_TSO6;
 
+	if (be_multi_rxq(adapter))
+		netdev->features |= NETIF_F_RXHASH;
+
 	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
 		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
-- 
1.7.1

Re: [RFC] be2net: add rxhash support

[Eric Dumazet]

Le vendredi 25 février 2011 à 11:44 -0600, Ajit Khaparde a écrit :
> > -----Original Message-----
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Sent: Thursday, February 24, 2011 2:25 PM
> > To: Khaparde, Ajit
> > Cc: netdev@vger.kernel.org
> > Subject: [RFC] be2net: add rxhash support
> > 
> > Ajit, it seems be2net provides RSS hash value in rx compl descriptor ?
> > 
> > Could we feed skb->rxhash with it ?
> > 
> > Thanks !
> Thanks Eric. Sure.
> This is a long pending change which fell through the cracks.
> But then because hashing is enabled in the device only when
> Number of Rx Queues is > 1, I would suggest the following patch.
> 
> Unaware of exact conventions, I have added signed-off-by to the patch already.
> 
> Thanks
> 
> -----
> [PATCH net-next] be2net: add rxhash support
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde@emulex.com>
> ---
>  drivers/net/benet/be_main.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> index 26f9c56..8c4b782 100644
> --- a/drivers/net/benet/be_main.c
> +++ b/drivers/net/benet/be_main.c
> @@ -1038,6 +1038,10 @@ static void be_rx_compl_process(struct be_adapter *adapter,
>  
>  	skb->truesize = skb->len + sizeof(struct sk_buff);
>  	skb->protocol = eth_type_trans(skb, adapter->netdev);
> +	if (adapter->netdev->features & NETIF_F_RXHASH)
> +		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
> +					rsshash, rxcp);
> +
>  
>  	vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp);
>  	vtm = AMAP_GET_BITS(struct amap_eth_rx_compl, vtm, rxcp);
> @@ -1099,6 +1103,10 @@ static void be_rx_compl_process_gro(struct be_adapter *adapter,
>  		return;
>  	}
>  
> +	if (adapter->netdev->features & NETIF_F_RXHASH)
> +		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
> +						rsshash, rxcp);
> +
>  	remaining = pkt_size;
>  	for (i = 0, j = -1; i < num_rcvd; i++) {
>  		page_info = get_rx_page_info(adapter, rxo, rxq_idx);
> @@ -2619,6 +2627,9 @@ static void be_netdev_init(struct net_device *netdev)
>  		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  		NETIF_F_GRO | NETIF_F_TSO6;
>  
> +	if (be_multi_rxq(adapter))
> +		netdev->features |= NETIF_F_RXHASH;
> +
>  	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
>  		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>  


I added some traces, and I am not sure its OK :

With one active tcp flow, I got different rxhash values :

[ 1064.674253] rxhash=bbd37952 rsshp=1 bank=1
[ 1064.738104] rxhash=37acd31d rsshp=1 bank=1
[ 1064.741684] rxhash=bbd37952 rsshp=1 bank=1
[ 1064.874283] rxhash=bbd37952 rsshp=1 bank=1
[ 1064.940201] rxhash=bbd37952 rsshp=1 bank=1
[ 1064.955278] rxhash=b668ace2 rsshp=1 bank=1
[ 1065.080028] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.153360] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.293164] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.401862] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.460506] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.519980] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.650160] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.717585] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.730909] rxhash=37acd31d rsshp=1 bank=1
[ 1065.840350] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.900704] rxhash=bbd37952 rsshp=1 bank=1
[ 1065.931526] rxhash=b668ace2 rsshp=1 bank=1
[ 1066.503657] rxhash=bbd37952 rsshp=1 bank=1
[ 1066.570138] rxhash=bbd37952 rsshp=1 bank=1

How is it possible ?

(I have a VLAN config on top of a bonding)

Re: [RFC] be2net: add rxhash support

[Eric Dumazet]

Le vendredi 25 février 2011 à 19:21 +0100, Eric Dumazet a écrit :
> Le vendredi 25 février 2011 à 11:44 -0600, Ajit Khaparde a écrit :
> > -----
> > [PATCH net-next] be2net: add rxhash support
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Ajit Khaparde <ajit.khaparde@emulex.com>
> > ---
> >  drivers/net/benet/be_main.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> > index 26f9c56..8c4b782 100644
> > --- a/drivers/net/benet/be_main.c
> > +++ b/drivers/net/benet/be_main.c
> > @@ -1038,6 +1038,10 @@ static void be_rx_compl_process(struct be_adapter *adapter,
> >  
> >  	skb->truesize = skb->len + sizeof(struct sk_buff);
> >  	skb->protocol = eth_type_trans(skb, adapter->netdev);
> > +	if (adapter->netdev->features & NETIF_F_RXHASH)
> > +		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
> > +					rsshash, rxcp);
> > +
> >  
> >  	vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp);
> >  	vtm = AMAP_GET_BITS(struct amap_eth_rx_compl, vtm, rxcp);
> > @@ -1099,6 +1103,10 @@ static void be_rx_compl_process_gro(struct be_adapter *adapter,
> >  		return;
> >  	}
> >  
> > +	if (adapter->netdev->features & NETIF_F_RXHASH)
> > +		skb->rxhash = AMAP_GET_BITS(struct amap_eth_rx_compl,
> > +						rsshash, rxcp);
> > +
> >  	remaining = pkt_size;
> >  	for (i = 0, j = -1; i < num_rcvd; i++) {
> >  		page_info = get_rx_page_info(adapter, rxo, rxq_idx);
> > @@ -2619,6 +2627,9 @@ static void be_netdev_init(struct net_device *netdev)
> >  		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> >  		NETIF_F_GRO | NETIF_F_TSO6;
> >  
> > +	if (be_multi_rxq(adapter))
> > +		netdev->features |= NETIF_F_RXHASH;
> > +
> >  	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
> >  		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> >  
> 
> 
> I added some traces, and I am not sure its OK :
> 
> With one active tcp flow, I got different rxhash values :
> 
> [ 1064.674253] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.738104] rxhash=37acd31d rsshp=1 bank=1
> [ 1064.741684] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.874283] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.940201] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.955278] rxhash=b668ace2 rsshp=1 bank=1
> [ 1065.080028] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.153360] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.293164] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.401862] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.460506] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.519980] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.650160] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.717585] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.730909] rxhash=37acd31d rsshp=1 bank=1
> [ 1065.840350] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.900704] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.931526] rxhash=b668ace2 rsshp=1 bank=1
> [ 1066.503657] rxhash=bbd37952 rsshp=1 bank=1
> [ 1066.570138] rxhash=bbd37952 rsshp=1 bank=1
> 
> How is it possible ?
> 
> (I have a VLAN config on top of a bonding)
> 
> 

Also, Ajit, we need something to allow ethtool -K rxhash {on|off}

Something like (completely untested)

diff --git a/drivers/net/benet/be_ethtool.c b/drivers/net/benet/be_ethtool.c
index 6e5e433..540c1ea 100644
--- a/drivers/net/benet/be_ethtool.c
+++ b/drivers/net/benet/be_ethtool.c
@@ -712,6 +712,11 @@ be_read_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom,
 	return status;
 }
 
+static int be_set_flags(struct net_device *dev, u32 data)
+{
+	return ethtool_op_set_flags(dev, data, ETH_FLAG_RXHASH);
+}
+
 const struct ethtool_ops be_ethtool_ops = {
 	.get_settings = be_get_settings,
 	.get_drvinfo = be_get_drvinfo,
@@ -739,4 +744,5 @@ const struct ethtool_ops be_ethtool_ops = {
 	.get_ethtool_stats = be_get_ethtool_stats,
 	.flash_device = be_do_flash,
 	.self_test = be_self_test,
+	.set_flags = be_set_flags,
 };

Re: [RFC] be2net: add rxhash support

[Ajit Khaparde]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Friday, February 25, 2011 1:33 PM
> To: Khaparde, Ajit
> Cc: netdev@vger.kernel.org
> Subject: Re: [RFC] be2net: add rxhash support

> I added some traces, and I am not sure its OK :
> 
> With one active tcp flow, I got different rxhash values :
> 
> [ 1064.674253] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.738104] rxhash=37acd31d rsshp=1 bank=1
> [ 1064.741684] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.874283] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.940201] rxhash=bbd37952 rsshp=1 bank=1
> [ 1064.955278] rxhash=b668ace2 rsshp=1 bank=1
> [ 1065.080028] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.153360] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.293164] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.401862] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.460506] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.519980] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.650160] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.717585] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.730909] rxhash=37acd31d rsshp=1 bank=1
> [ 1065.840350] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.900704] rxhash=bbd37952 rsshp=1 bank=1
> [ 1065.931526] rxhash=b668ace2 rsshp=1 bank=1
> [ 1066.503657] rxhash=bbd37952 rsshp=1 bank=1
> [ 1066.570138] rxhash=bbd37952 rsshp=1 bank=1
> 
> How is it possible ?
> 
> (I have a VLAN config on top of a bonding)
> 
I'm looking at this..
There is no switch involved in your test, just back to back?

> 
> Also, Ajit, we need something to allow ethtool -K rxhash {on|off}
> 
> Something like (completely untested)

Yes. That is in the works.

Thanks
-Ajit

Re: [RFC] be2net: add rxhash support

[Eric Dumazet]

Le vendredi 25 février 2011 à 13:36 -0600, Ajit Khaparde a écrit :
> > -----Original Message-----
> > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > 
> > How is it possible ?
> > 
> > (I have a VLAN config on top of a bonding)
> > 
> I'm looking at this..
> There is no switch involved in your test, just back to back?
> 

There is a switch.

Machines are HP ProLiant BL460c G7

But why do you ask ?

Re: [RFC] be2net: add rxhash support

[Ajit Khaparde]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Friday, February 25, 2011 1:45 PM
> To: Khaparde, Ajit
> Cc: netdev@vger.kernel.org
> Subject: Re: [RFC] be2net: add rxhash support
> 
> Le vendredi 25 février 2011 à 13:36 -0600, Ajit Khaparde a écrit :
> > > -----Original Message-----
> > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > >
> > > How is it possible ?
> > >
> > > (I have a VLAN config on top of a bonding)
> > >
> > I'm looking at this..
> > There is no switch involved in your test, just back to back?
> >
> 
> There is a switch.
> 
> Machines are HP ProLiant BL460c G7
> 
> But why do you ask ?
> 

I asked that because, if a switch is part a of the configuration,
the ASIC can receive packets other than the tcp flow.

And if hashing is enabled for IP packets, we can see this behavior.
The other values indicate that hashing has been enabled for IPv4 packets.

Thanks
-Ajit

Re: [RFC] be2net: add rxhash support

[Eric Dumazet]

Le vendredi 25 février 2011 à 15:35 -0600, Ajit Khaparde a écrit :

> I asked that because, if a switch is part a of the configuration,
> the ASIC can receive packets other than the tcp flow.
> 
> And if hashing is enabled for IP packets, we can see this behavior.
> The other values indicate that hashing has been enabled for IPv4 packets.

To make sure RSS (and rxhash) was OK, I added following debugging aid :

diff --git a/include/net/sock.h b/include/net/sock.h
index da0534d..e9b1180 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -688,6 +688,7 @@ static inline void sock_rps_save_rxhash(struct sock *sk, u32 rxhash)
 {
 #ifdef CONFIG_RPS
 	if (unlikely(sk->sk_rxhash != rxhash)) {
+		pr_err("rxhash change from %x to %x\n", sk->sk_rxhash, rxhash);
 		sock_rps_reset_flow(sk);
 		sk->sk_rxhash = rxhash;
 	}


And got following traces :

[  201.170297] change rxhash from 0 to be0b5a87
[  232.607474] bonding: bond1: Setting eth3 as active slave.
[  232.607478] bonding: bond1: making interface eth3 the new active one.
[  232.710848] change rxhash from be0b5a87 to e56a3c1e
[  300.047500] bonding: bond1: Setting eth1 as active slave.
[  300.047504] bonding: bond1: making interface eth1 the new active one.
[  300.159162] change rxhash from e56a3c1e to be0b5a87

The flip occured when I changed my active slave (bonding mode=1).

eth1 is a bnx2 NIC, while eth3 a be2net one, so its OK to change the rxhash in this case 
(different firmware/algo)

So as far as be2net is concerned, everything seems OK : all packets for 
a given flow get an unique RSS hash and can feed skb->rxhash

Re: [RFC] be2net: add rxhash support

[Ajit Khaparde]

> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Saturday, February 26, 2011 4:31 AM
> To: Khaparde, Ajit
> Cc: netdev@vger.kernel.org
> Subject: Re: [RFC] be2net: add rxhash support
> 
> Le vendredi 25 février 2011 à 15:35 -0600, Ajit Khaparde a écrit :
> 
> > I asked that because, if a switch is part a of the configuration,
> > the ASIC can receive packets other than the tcp flow.
> >
> > And if hashing is enabled for IP packets, we can see this behavior.
> > The other values indicate that hashing has been enabled for IPv4
> packets.
> 
> To make sure RSS (and rxhash) was OK, I added following debugging aid :
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index da0534d..e9b1180 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -688,6 +688,7 @@ static inline void sock_rps_save_rxhash(struct sock
> *sk, u32 rxhash)
>  {
>  #ifdef CONFIG_RPS
>  	if (unlikely(sk->sk_rxhash != rxhash)) {
> +		pr_err("rxhash change from %x to %x\n", sk->sk_rxhash,
> rxhash);
>  		sock_rps_reset_flow(sk);
>  		sk->sk_rxhash = rxhash;
>  	}
> 
> 
> And got following traces :
> 
> [  201.170297] change rxhash from 0 to be0b5a87
> [  232.607474] bonding: bond1: Setting eth3 as active slave.
> [  232.607478] bonding: bond1: making interface eth3 the new active
> one.
> [  232.710848] change rxhash from be0b5a87 to e56a3c1e
> [  300.047500] bonding: bond1: Setting eth1 as active slave.
> [  300.047504] bonding: bond1: making interface eth1 the new active
> one.
> [  300.159162] change rxhash from e56a3c1e to be0b5a87
> 
> The flip occured when I changed my active slave (bonding mode=1).
> 
> eth1 is a bnx2 NIC, while eth3 a be2net one, so its OK to change the
> rxhash in this case
> (different firmware/algo)
> 
> So as far as be2net is concerned, everything seems OK : all packets for
> a given flow get an unique RSS hash and can feed skb->rxhash
> 
Fair enough. Thanks.
I guess a fresh patch with the ethtool support included will be ideal,
instead of the previous patch?

-Ajit