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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8628FE4E7912BF47A96AE7DD7BAC0AADDDE6470528@SJEXCHCCR02.corp.ad.broadcom.com>
Date:	Sun, 11 Jul 2010 11:18:02 -0700
From:	"Vladislav Zolotarov" <vladz@...adcom.com>
To:	"Eric Dumazet" <eric.dumazet@...il.com>
cc:	"David Miller" <davem@...emloft.net>,
	"therbert@...gle.com" <therbert@...gle.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] bnx2x: add support for receive hashing



> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> Sent: Sunday, July 11, 2010 8:42 PM
> To: Vladislav Zolotarov
> Cc: David Miller; therbert@...gle.com; netdev@...r.kernel.org
> Subject: RE: [PATCH] bnx2x: add support for receive hashing
> 
> Le dimanche 11 juillet 2010 à 10:22 -0700, Vladislav Zolotarov a écrit :
> >
> > > -----Original Message-----
> > > From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
> On
> > > Behalf Of Eric Dumazet
> > > Sent: Sunday, July 11, 2010 7:45 PM
> > > To: Vladislav Zolotarov
> > > Cc: David Miller; therbert@...gle.com; netdev@...r.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.

Ok. No, prob. In that case we just need to check the netdev->feature in
the bnx2x_rx_int(). Checking on CQE flags is a not needed.
I'll post a patch shortly. 

Thanks,
vlad

> 
> 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.
> 
> 
>