RE: [PATCH] bnx2x: add support for receive hashing

[Eric Dumazet <eric.dumazet@gmail.com>]

Le dimanche 11 juillet 2010 à 10:22 -0700, Vladislav Zolotarov a écrit :
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Eric Dumazet
> > Sent: Sunday, July 11, 2010 7:45 PM
> > To: Vladislav Zolotarov
> > Cc: David Miller; therbert@google.com; netdev@vger.kernel.org
> > Subject: RE: [PATCH] bnx2x: add support for receive hashing
> > 
> > Le dimanche 11 juillet 2010 à 06:16 -0700, Vladislav Zolotarov a écrit :
> > > Dave, it's obvious that there a demand for a new HW/FW configuration
> > > from our side - "rx hash enable" which is currently tightly coupled
> > > with the RSS capability. As long as RSS flow in our FW includes a few
> > > more things apart from just creating an RSS hash and as long as there
> > > are flows (even hypothetical) that demand the RSS hash regardless the
> > > RSS itself we started to work on separation of these two features from
> > > FW perspective. This of course will demand a new FW version but once
> > > we have it we'll be able to be more specific in HW configuration and
> > > have a cleaner code.
> > >
> > > Technically, our FW may provide the Rx HASH always and in a current
> > > driver configuration this is what it does.
> > > I wonder if the driver always provides the HW RX HASH in the
> > > skb->rxhash regardless the value of NETIF_F_RXHASH bit in a
> > > netdev->features will it cause any harm? If not we can get rid of two
> > > extra conditionals in the bnx2x_rx_int().
> > 
> > Hi
> > 
> > Please dont top-post on netdev, thanks.
> 
> This discussion is directly related to Tom's patch that's why I'm posting on this thread. 
> 

Thats not the question. I am saying "dont top-post", not "dont change
the subject"

> > 
> > NETIF_RX_HASH bit is needed so that we can disable skb->rxhash from a
> > particular NIC if we know the hardware provided rxhash is not fulfilling
> > our needs (We prefer spend some cpu cycles to recompute a software
> > rxhash).
> > 
> > Software one for example deliver same rxhash values for both ways of a
> > TCP flow, it can help conntracking for example.
> 
> I understand that, in that case the proper implementation would be to check the netdev->features when u decide to calculate the SW rxhash, isn't it?
> 

Nope, please check get_rps_cpus()

We only do :

if (skb->rxhash)
	goto got_hash; /* Skip hash computation on packet header */

That means if skb->rxhash is set, we dont force a recompute.

Your suggestion would move a test from device driver into
get_rps_cpus() ?

Given RPS is more targeted to old devices (not able to provide rxhash),
I am not sure it brings anything.

> > 
> > The conditional in driver rx is cheap, since the condition is the same
> > for all packets and CPU can predicts the branch.
> 
> Not exactly. Our FW/HW won't provide the rxhash for none-IP packets
> setting the hash CQE field to zero and clearing the
> ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG in the CQE statsu_flags (for ARPs
> for instance). Branch prediction is nice but considering my previous
> remark why do we need this branch at all?

Please limit your lines to 70 chars

We want drivers to set skb->rxhash only if allowed to.

If you feel this is bad for your firmware/hardware/driver, dont set
skb->rxhash.