[PATCH net-next] cxgb4: set skb->rxhash

[PATCH net-next] cxgb4: set skb->rxhash

[Dimitris Michailidis]

Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
set skb->rxhash to the HW calculated hash accordingly.

Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
---
 drivers/net/cxgb4/cxgb4_main.c |   15 ++++++++++++++-
 drivers/net/cxgb4/sge.c        |    7 ++++++-
 drivers/net/cxgb4/t4_msg.h     |    1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/cxgb4/cxgb4_main.c b/drivers/net/cxgb4/cxgb4_main.c
index 5f582db..1bad500 100644
--- a/drivers/net/cxgb4/cxgb4_main.c
+++ b/drivers/net/cxgb4/cxgb4_main.c
@@ -1711,6 +1711,18 @@ static int set_tso(struct net_device *dev, u32 value)
 	return 0;
 }
 
+static int set_flags(struct net_device *dev, u32 flags)
+{
+	if (flags & ~ETH_FLAG_RXHASH)
+		return -EOPNOTSUPP;
+
+	if (flags & ETH_FLAG_RXHASH)
+		dev->features |= NETIF_F_RXHASH;
+	else
+		dev->features &= ~NETIF_F_RXHASH;
+	return 0;
+}
+
 static struct ethtool_ops cxgb_ethtool_ops = {
 	.get_settings      = get_settings,
 	.set_settings      = set_settings,
@@ -1741,6 +1753,7 @@ static struct ethtool_ops cxgb_ethtool_ops = {
 	.get_wol           = get_wol,
 	.set_wol           = set_wol,
 	.set_tso           = set_tso,
+	.set_flags         = set_flags,
 	.flash_device      = set_flash,
 };
 
@@ -3203,7 +3216,7 @@ static int __devinit init_one(struct pci_dev *pdev,
 
 		netdev->features |= NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6;
 		netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-		netdev->features |= NETIF_F_GRO | highdma;
+		netdev->features |= NETIF_F_GRO | NETIF_F_RXHASH | highdma;
 		netdev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
 		netdev->vlan_features = netdev->features & VLAN_FEAT;
 
diff --git a/drivers/net/cxgb4/sge.c b/drivers/net/cxgb4/sge.c
index 65d91c4..e804ffc 100644
--- a/drivers/net/cxgb4/sge.c
+++ b/drivers/net/cxgb4/sge.c
@@ -1524,6 +1524,8 @@ static void do_gro(struct sge_eth_rxq *rxq, const struct pkt_gl *gl,
 	skb->truesize += skb->data_len;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 	skb_record_rx_queue(skb, rxq->rspq.idx);
+	if (rxq->rspq.netdev->features & NETIF_F_RXHASH)
+		skb->rxhash = ntohl(pkt->rsshdr.hash_val);
 
 	if (unlikely(pkt->vlan_ex)) {
 		struct port_info *pi = netdev_priv(rxq->rspq.netdev);
@@ -1565,7 +1567,7 @@ int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 	if (unlikely(*(u8 *)rsp == CPL_TRACE_PKT))
 		return handle_trace_pkt(q->adap, si);
 
-	pkt = (void *)&rsp[1];
+	pkt = (const struct cpl_rx_pkt *)rsp;
 	csum_ok = pkt->csum_calc && !pkt->err_vec;
 	if ((pkt->l2info & htonl(RXF_TCP)) &&
 	    (q->netdev->features & NETIF_F_GRO) && csum_ok && !pkt->ip_frag) {
@@ -1583,6 +1585,9 @@ int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 	__skb_pull(skb, RX_PKT_PAD);      /* remove ethernet header padding */
 	skb->protocol = eth_type_trans(skb, q->netdev);
 	skb_record_rx_queue(skb, q->idx);
+	if (skb->dev->features & NETIF_F_RXHASH)
+		skb->rxhash = ntohl(pkt->rsshdr.hash_val);
+
 	pi = netdev_priv(skb->dev);
 	rxq->stats.pkts++;
 
diff --git a/drivers/net/cxgb4/t4_msg.h b/drivers/net/cxgb4/t4_msg.h
index fdb1174..7a981b8 100644
--- a/drivers/net/cxgb4/t4_msg.h
+++ b/drivers/net/cxgb4/t4_msg.h
@@ -503,6 +503,7 @@ struct cpl_rx_data_ack {
 };
 
 struct cpl_rx_pkt {
+	struct rss_header rsshdr;
 	u8 opcode;
 #if defined(__LITTLE_ENDIAN_BITFIELD)
 	u8 iff:4;
-- 
1.5.4

Re: [PATCH net-next] cxgb4: set skb->rxhash

[Stephen Hemminger]

On Tue, 27 Apr 2010 15:20:25 -0700
Dimitris Michailidis <dm@chelsio.com> wrote:

> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
> set skb->rxhash to the HW calculated hash accordingly.
> 
> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
> ---
>  drivers/net/cxgb4/cxgb4_main.c |   15 ++++++++++++++-
>  drivers/net/cxgb4/sge.c        |    7 ++++++-
>  drivers/net/cxgb4/t4_msg.h     |    1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/cxgb4/cxgb4_main.c b/drivers/net/cxgb4/cxgb4_main.c
> index 5f582db..1bad500 100644
> --- a/drivers/net/cxgb4/cxgb4_main.c
> +++ b/drivers/net/cxgb4/cxgb4_main.c
> @@ -1711,6 +1711,18 @@ static int set_tso(struct net_device *dev, u32 value)
>  	return 0;
>  }
>  
> +static int set_flags(struct net_device *dev, u32 flags)
> +{
> +	if (flags & ~ETH_FLAG_RXHASH)
> +		return -EOPNOTSUPP;
> +
> +	if (flags & ETH_FLAG_RXHASH)
> +		dev->features |= NETIF_F_RXHASH;
> +	else
> +		dev->features &= ~NETIF_F_RXHASH;
> +	return 0;
> +}

You need to check for and reject other values NTUPLE and LRO

Re: [PATCH net-next] cxgb4: set skb->rxhash

[Dimitris Michailidis]

Stephen Hemminger wrote:
> On Tue, 27 Apr 2010 15:20:25 -0700
> Dimitris Michailidis <dm@chelsio.com> wrote:
> 
>> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
>> set skb->rxhash to the HW calculated hash accordingly.
>>
>> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
>> ---
>>  drivers/net/cxgb4/cxgb4_main.c |   15 ++++++++++++++-
>>  drivers/net/cxgb4/sge.c        |    7 ++++++-
>>  drivers/net/cxgb4/t4_msg.h     |    1 +
>>  3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/cxgb4/cxgb4_main.c b/drivers/net/cxgb4/cxgb4_main.c
>> index 5f582db..1bad500 100644
>> --- a/drivers/net/cxgb4/cxgb4_main.c
>> +++ b/drivers/net/cxgb4/cxgb4_main.c
>> @@ -1711,6 +1711,18 @@ static int set_tso(struct net_device *dev, u32 value)
>>  	return 0;
>>  }
>>  
>> +static int set_flags(struct net_device *dev, u32 flags)
>> +{
>> +	if (flags & ~ETH_FLAG_RXHASH)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (flags & ETH_FLAG_RXHASH)
>> +		dev->features |= NETIF_F_RXHASH;
>> +	else
>> +		dev->features &= ~NETIF_F_RXHASH;
>> +	return 0;
>> +}
> 
> You need to check for and reject other values NTUPLE and LRO

The first if does that, doesn't it?

Re: [PATCH net-next] cxgb4: set skb->rxhash

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 27 Apr 2010 15:36:56 -0700

>> +static int set_flags(struct net_device *dev, u32 flags)
>> +{
>> +	if (flags & ~ETH_FLAG_RXHASH)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (flags & ETH_FLAG_RXHASH)
>> +		dev->features |= NETIF_F_RXHASH;
>> +	else
>> +		dev->features &= ~NETIF_F_RXHASH;
>> +	return 0;
>> +}
> 
> You need to check for and reject other values NTUPLE and LRO

He does, it's the first he does in the function.

Re: [PATCH net-next] cxgb4: set skb->rxhash

[Eric Dumazet]

Le mardi 27 avril 2010 à 15:20 -0700, Dimitris Michailidis a écrit :
> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
> set skb->rxhash to the HW calculated hash accordingly.
> 
> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>

>  	skb_record_rx_queue(skb, rxq->rspq.idx);
> +	if (rxq->rspq.netdev->features & NETIF_F_RXHASH)
> +		skb->rxhash = ntohl(pkt->rsshdr.hash_val);
>  

Its probably minor, but hash_val being a semi random 32bits value, you
can avoid the ntohl() conversion, using a (__force u32) cast ...

Re: [PATCH net-next] cxgb4: set skb->rxhash

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 28 Apr 2010 00:43:12 +0200

> Le mardi 27 avril 2010 à 15:20 -0700, Dimitris Michailidis a écrit :
>> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
>> set skb->rxhash to the HW calculated hash accordingly.
>> 
>> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
> 
>>  	skb_record_rx_queue(skb, rxq->rspq.idx);
>> +	if (rxq->rspq.netdev->features & NETIF_F_RXHASH)
>> +		skb->rxhash = ntohl(pkt->rsshdr.hash_val);
>>  
> 
> Its probably minor, but hash_val being a semi random 32bits value, you
> can avoid the ntohl() conversion, using a (__force u32) cast ...

Agreed.

Re: [PATCH net-next] cxgb4: set skb->rxhash

[David Miller]

From: Dimitris Michailidis <dm@chelsio.com>
Date: Tue, 27 Apr 2010 15:20:25 -0700

> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
> set skb->rxhash to the HW calculated hash accordingly.
> 
> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>

Does the CXGB4 hash the ports for non-TCP packets?

If not, you will need to check the packet type (preferrably using
RX descriptor information rather than actually parsing the packet)
and only record the ->rxhash for TCP packets.

Re: [PATCH net-next] cxgb4: set skb->rxhash

[Dimitris Michailidis]

David Miller wrote:
> From: Dimitris Michailidis <dm@chelsio.com>
> Date: Tue, 27 Apr 2010 15:20:25 -0700
> 
>> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
>> set skb->rxhash to the HW calculated hash accordingly.
>>
>> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
> 
> Does the CXGB4 hash the ports for non-TCP packets?
> 
> If not, you will need to check the packet type (preferrably using
> RX descriptor information rather than actually parsing the packet)
> and only record the ->rxhash for TCP packets.
> 

It does, optionally.  Someday I need to implement ->set_rxnfc to expose the 
few available options but it's on by default.

Re: [PATCH net-next] cxgb4: set skb->rxhash

[David Miller]

From: Dimitris Michailidis <dm@chelsio.com>
Date: Tue, 27 Apr 2010 15:46:45 -0700

> David Miller wrote:
>> From: Dimitris Michailidis <dm@chelsio.com>
>> Date: Tue, 27 Apr 2010 15:20:25 -0700
>> 
>>> Implement the ->set_flags ethtool method to control NETIF_F_RXHASH and
>>> set skb->rxhash to the HW calculated hash accordingly.
>>>
>>> Signed-off-by: Dimitris Michailidis <dm@chelsio.com>
>> Does the CXGB4 hash the ports for non-TCP packets?
>> If not, you will need to check the packet type (preferrably using
>> RX descriptor information rather than actually parsing the packet)
>> and only record the ->rxhash for TCP packets.
>> 
> 
> It does, optionally.  Someday I need to implement ->set_rxnfc to
> expose the few available options but it's on by default.

Great, if you could respin this patch and ditch the ntohl() as Eric
suggested I'll apply it.

Thanks!