[PATCH] bnx2x: add support for receive hashing

[PATCH] bnx2x: add support for receive hashing

[Tom Herbert]

Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
for use in skb->rxhash.

Signed-off-by: Tom Herbert <therbert@google.com>
---
diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
index 0819530..8bd2368 100644
--- a/drivers/net/bnx2x.h
+++ b/drivers/net/bnx2x.h
@@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg, u32 expected, int ms,
 		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
 		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
 
-#define MULTI_FLAGS(bp) \
+#define RSS_FLAGS(bp) \
 		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
 		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
 		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 0c6dba2..613f727 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 		struct sw_rx_bd *rx_buf = NULL;
 		struct sk_buff *skb;
 		union eth_rx_cqe *cqe;
-		u8 cqe_fp_flags;
+		u8 cqe_fp_flags, cqe_fp_status_flags;
 		u16 len, pad;
 
 		comp_ring_cons = RCQ_BD(sw_comp_cons);
@@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 
 		cqe = &fp->rx_comp_ring[comp_ring_cons];
 		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
+		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
 
 		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
 		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
@@ -1727,6 +1728,12 @@ reuse_rx:
 
 			skb->protocol = eth_type_trans(skb, bp->dev);
 
+			if ((bp->dev->features & ETH_FLAG_RXHASH) &&
+			    (cqe_fp_status_flags &
+			     ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
+				skb->rxhash = le32_to_cpu(
+				    cqe->fast_path_cqe.rss_hash_result);
+
 			skb->ip_summed = CHECKSUM_NONE;
 			if (bp->rx_csum) {
 				if (likely(BNX2X_RX_CSUM_OK(cqe)))
@@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x *bp)
 	u32 offset;
 	u16 max_agg_size;
 
-	if (is_multi(bp)) {
-		tstorm_config.config_flags = MULTI_FLAGS(bp);
+	tstorm_config.config_flags = RSS_FLAGS(bp);
+
+	if (is_multi(bp))
 		tstorm_config.rss_result_mask = MULTI_MASK;
-	}
 
 	/* Enable TPA if needed */
 	if (bp->flags & TPA_ENABLE_FLAG)
@@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
 	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
 
 	REG_WR(bp, SRC_REG_SOFT_RST, 1);
-	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
-		REG_WR(bp, i, 0xc0cac01a);
-		/* TODO: replace with something meaningful */
-	}
+	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
+		REG_WR(bp, i, random32());
 	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
 #ifdef BCM_CNIC
 	REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
@@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev, u32 data)
 		changed = 1;
 	}
 
+	if (data & ETH_FLAG_RXHASH)
+		dev->features |= NETIF_F_RXHASH;
+	else
+		dev->features &= ~NETIF_F_RXHASH;
+
 	if (changed && netif_running(dev)) {
 		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
 		rc = bnx2x_nic_load(bp, LOAD_NORMAL);

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

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Thu, 22 Apr 2010 22:54:16 -0700 (PDT)

> Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
> for use in skb->rxhash.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>

Sweeeeet.

Applied, thanks Tom.

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

[Eric Dumazet]

Le jeudi 22 avril 2010 à 22:54 -0700, Tom Herbert a écrit :
> Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
> for use in skb->rxhash.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> index 0819530..8bd2368 100644
> --- a/drivers/net/bnx2x.h
> +++ b/drivers/net/bnx2x.h
> @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg, u32 expected, int ms,
>  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
>  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
>  
> -#define MULTI_FLAGS(bp) \
> +#define RSS_FLAGS(bp) \
>  		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
>  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
>  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 0c6dba2..613f727 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
>  		struct sw_rx_bd *rx_buf = NULL;
>  		struct sk_buff *skb;
>  		union eth_rx_cqe *cqe;
> -		u8 cqe_fp_flags;
> +		u8 cqe_fp_flags, cqe_fp_status_flags;
>  		u16 len, pad;
>  
>  		comp_ring_cons = RCQ_BD(sw_comp_cons);
> @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
>  
>  		cqe = &fp->rx_comp_ring[comp_ring_cons];
>  		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
> +		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
>  
>  		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
>  		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
> @@ -1727,6 +1728,12 @@ reuse_rx:
>  
>  			skb->protocol = eth_type_trans(skb, bp->dev);
>  
> +			if ((bp->dev->features & ETH_FLAG_RXHASH) &&
> +			    (cqe_fp_status_flags &
> +			     ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
> +				skb->rxhash = le32_to_cpu(
> +				    cqe->fast_path_cqe.rss_hash_result);
> +
>  			skb->ip_summed = CHECKSUM_NONE;
>  			if (bp->rx_csum) {
>  				if (likely(BNX2X_RX_CSUM_OK(cqe)))
> @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x *bp)
>  	u32 offset;
>  	u16 max_agg_size;
>  
> -	if (is_multi(bp)) {
> -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> +	tstorm_config.config_flags = RSS_FLAGS(bp);
> +
> +	if (is_multi(bp))
>  		tstorm_config.rss_result_mask = MULTI_MASK;
> -	}
>  
>  	/* Enable TPA if needed */
>  	if (bp->flags & TPA_ENABLE_FLAG)
> @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
>  	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
>  
>  	REG_WR(bp, SRC_REG_SOFT_RST, 1);
> -	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> -		REG_WR(bp, i, 0xc0cac01a);
> -		/* TODO: replace with something meaningful */
> -	}
> +	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> +		REG_WR(bp, i, random32());
>  	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
>  #ifdef BCM_CNIC
>  	REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
> @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev, u32 data)
>  		changed = 1;
>  	}
>  
> +	if (data & ETH_FLAG_RXHASH)
> +		dev->features |= NETIF_F_RXHASH;
> +	else
> +		dev->features &= ~NETIF_F_RXHASH;
> +
>  	if (changed && netif_running(dev)) {
>  		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
>  		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> --

Hi Tom

I tested this rxhash feature on my bnx2x card, using latest net-next-2.6
and appropriate ethtool

ethtool -k eth1 rxhash on

Then I used my pktgen script, to flood machine with flows with udp dst
port varying between 4000 and 4015.

Software generated rxhash is OK (16 different values).

But with bnx2x provided hash, all skb were hashed to same rxhash
value :(

What are the specs of this hardware hash ?
It seems to not care of udp source/destination ports.

Also, should'nt we feed same values for the seeds on different nics ?

for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
	REG_WR(bp, i, random32());

Thanks

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

[Tom Herbert]

> Hi Tom
>
> I tested this rxhash feature on my bnx2x card, using latest net-next-2.6
> and appropriate ethtool
>
> ethtool -k eth1 rxhash on
>
> Then I used my pktgen script, to flood machine with flows with udp dst
> port varying between 4000 and 4015.
>
> Software generated rxhash is OK (16 different values).
>
> But with bnx2x provided hash, all skb were hashed to same rxhash
> value :(
>
> What are the specs of this hardware hash ?
> It seems to not care of udp source/destination ports.
>

It would appear that way :-(.  I was going to ping Broadcom folks to
see if there's support for UDP.

> Also, should'nt we feed same values for the seeds on different nics ?
>
> for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
>        REG_WR(bp, i, random32());
>

Yes, that is reasonable.

> Thanks
>
>
>

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

[Ben Hutchings]

On Mon, 2010-04-26 at 10:38 -0700, Tom Herbert wrote:
> > Hi Tom
> >
> > I tested this rxhash feature on my bnx2x card, using latest net-next-2.6
> > and appropriate ethtool
> >
> > ethtool -k eth1 rxhash on
> >
> > Then I used my pktgen script, to flood machine with flows with udp dst
> > port varying between 4000 and 4015.
> >
> > Software generated rxhash is OK (16 different values).
> >
> > But with bnx2x provided hash, all skb were hashed to same rxhash
> > value :(
> >
> > What are the specs of this hardware hash ?
> > It seems to not care of udp source/destination ports.
> >
> 
> It would appear that way :-(.  I was going to ping Broadcom folks to
> see if there's support for UDP.
[...]

Unfortunately the widely-implemented Toeplitz hash functions are defined
only for TCP/IPv4, TCP/IPv6, IPv4 and IPv6.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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] bnx2x: add support for receive hashing

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 26 Apr 2010 18:47:52 +0100

> Unfortunately the widely-implemented Toeplitz hash functions are defined
> only for TCP/IPv4, TCP/IPv6, IPv4 and IPv6.

What a complete and utter waste all of this is then.

Defining the hash as TCP only is about as stupid as making hw checksum
offload be TCP only.

Really, I'm completely stunned.

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

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Mon, 26 Apr 2010 10:38:27 -0700

> It would appear that way :-(.  I was going to ping Broadcom folks to
> see if there's support for UDP.

I'm pretty sure there isn't at this point.

We'll need to elide setting ->rxhash for non-TCP packets.  I bet that
the ETH_FAST_PATH_RX_CQE_RSS_HASH_TYPE field might be usable to making
this decision, but if not in the worst case we'll need to parse the
VLAN/ETH and IP4/IP6 headers to figure out the protocol.

Damn, I'm so pissed off about this.  This ruins everything!

How damn hard is it to add two 16-bit ports to the hash regardless of
protocol?

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

[Tom Herbert]

> I'm pretty sure there isn't at this point.
>
> We'll need to elide setting ->rxhash for non-TCP packets.  I bet that
> the ETH_FAST_PATH_RX_CQE_RSS_HASH_TYPE field might be usable to making
> this decision, but if not in the worst case we'll need to parse the
> VLAN/ETH and IP4/IP6 headers to figure out the protocol.
>
> Damn, I'm so pissed off about this.  This ruins everything!
>
> How damn hard is it to add two 16-bit ports to the hash regardless of
> protocol?
>
Fair question.

This also hits RSS/multiqueue. In a netperf RR test, 500 streams
between my two 16 core AMDs:  TCP 970K tps, UDP 370K tps.  I'm
surprised they didn't catch that in some benchmarks...

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

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Mon, 26 Apr 2010 11:19:05 -0700

> This also hits RSS/multiqueue. In a netperf RR test, 500 streams
> between my two 16 core AMDs:  TCP 970K tps, UDP 370K tps.  I'm
> surprised they didn't catch that in some benchmarks...

Meanwhile, these NIC vendors seem to have all the time in the world to
add iSCSI, RDMA and all the other stateful offload junk into their
firmware and silicon.

Yet they can't hash ports if the protocol is not TCP?  Beyond
baffling...

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

[Rick Jones]

David Miller wrote:
> From: Tom Herbert <therbert@google.com>
> Date: Mon, 26 Apr 2010 11:19:05 -0700
> 
> 
>>This also hits RSS/multiqueue. In a netperf RR test, 500 streams
>>between my two 16 core AMDs:  TCP 970K tps, UDP 370K tps.  I'm
>>surprised they didn't catch that in some benchmarks...
> 
> 
> Meanwhile, these NIC vendors seem to have all the time in the world to
> add iSCSI, RDMA and all the other stateful offload junk into their
> firmware and silicon.
> 
> Yet they can't hash ports if the protocol is not TCP?  Beyond
> baffling...

As a networking guy I can see why it seems baffling, but stepping out of myself 
and thinking like the customers with whom I've interacted over the years, it is 
not baffling at all.

By and large, customers do not do anything "substantial" with UDP.  NFS went to 
TCP mounts 99 times out of 10 many years ago, leaving DNS as about the only 
thing left*. So, customers will not be chomping at the bit for improved UDP 
scalability/performance.  They would though, be jumping up and down demanding 
iSCSI performance and by implication all that comes along for the ride.

rick jones

* And even there, one of the biggest pushes is trying to make TCP "transaction 
friendly" to deal with DNS messages becoming larger than typical MTUs.

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

[David Miller]

From: Rick Jones <rick.jones2@hp.com>
Date: Mon, 26 Apr 2010 13:19:31 -0700

> As a networking guy I can see why it seems baffling, but stepping out
> of myself and thinking like the customers with whom I've interacted
> over the years, it is not baffling at all.

<sarcasm>
And hey nobody is using SCTP either, that's right, nobody...
</sarcasm>

Look, don't try to defend this abomination of a situation with some
"customers only use TCP" argument.  It only makes the situation look
even more absurd.  

Furthermore, people test system scalability using tools like pktgen,
which surprise surprise generates streams of UDP packets.  Most
hardware based scalability testers spew UDP too.

Everything in the world points to "this toeplitz hash situation is
stupid an inexcusable."

If UDP isn't used by anyone, then you tell me why the checksum engines
of all of these chips can handle them just fine.  Maybe the guy who
works on the checksum logic blocks doesn't talk to the guy who works
on the hashing ones?  Maybe the checksum guy can find the ports in a
UDP packet, but the hashing dude can't locate them?

What the heck do you think people use for various forms of media
streaming?  They often use UDP and it has to scale, and they'd like to
move to DCCP at some point too which is another argument for a fully
protocol agnostic hash.

Why do you think Eric Dumazet gives a crap about UDP scalability and
is constantly testing it?  What about VOIP?  H.323, RTP, etc.?

Some of these cards can even statelessly offload UDP fragmentation
too, in silicon, not even in firmware.  What's their excuse for
screwing up the hash?

Look, this is a complete joke from every angle, at least admit that
fact.

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

[Rick Jones]

David Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 26 Apr 2010 13:19:31 -0700
> 
> 
>>As a networking guy I can see why it seems baffling, but stepping out
>>of myself and thinking like the customers with whom I've interacted
>>over the years, it is not baffling at all.
> 
> 
> <sarcasm>
> And hey nobody is using SCTP either, that's right, nobody...
> </sarcasm>
> 
> Look, don't try to defend this abomination of a situation with some
> "customers only use TCP" argument.  It only makes the situation look
> even more absurd.  

Do not confuse explanation with endorsement.

rick

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

[David Miller]

From: Rick Jones <rick.jones2@hp.com>
Date: Mon, 26 Apr 2010 13:48:22 -0700

> Do not confuse explanation with endorsement.

Ok, fair enough.

But I don't see even the "other perspective" argument being even
valid.  Big shops still use UDP and it has to scale.

Or have they made multicast magically start working with TCP so
they can us it to do trades on the NASDAQ?

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

[Rick Jones]

David Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 26 Apr 2010 13:48:22 -0700
> 
>>Do not confuse explanation with endorsement.
> 
> Ok, fair enough.
> 
> But I don't see even the "other perspective" argument being even
> valid.  Big shops still use UDP and it has to scale.

Preface - I too think it is massively stupid to ignore anything but TCP/IPv4, 
and unwise to ignore IPv6 and so on, but there is a very real reason why one of 
my email signatures reads:

"The road to hell is paved with business decisions"

> Or have they made multicast magically start working with TCP so
> they can us it to do trades on the NASDAQ?

No. How many NIC chips can NASDAQ be expected to move? 0.1%? or even 1% of the 
NIC chip market?

How many more NIC chips are in places where someone says "You sold me on 
iSCSI/FCoE/whatnot, why can't I get 'link-rate'  to/from iSCSI storage/whatnot?!"

The NIC designer is there with his finance guys breathing down his neck shouting 
"ROI Uber Alles!" and "Your budget is only this many monetary units!"  The 
system designers at the system vendors are hearing the same things from their 
own finance guys, have certain schedules, which then has them going to the NIC 
firms, who want to sell chips to the system guys "You have to be ready to ship 
by this date and your chip has to sell for no more than this."

Lather, rinse, repeat a few times and you get compromises on top of compromises.

Sometimes I think it is a wonder any of it actually works at all...

rick jones

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

[Eilon Greenstein]

On Mon, 2010-04-26 at 14:12 -0700, Rick Jones wrote:
> David Miller wrote:
> > From: Rick Jones <rick.jones2@hp.com>
> > Date: Mon, 26 Apr 2010 13:48:22 -0700
> > 
> >>Do not confuse explanation with endorsement.
> > 
> > Ok, fair enough.
> > 
> > But I don't see even the "other perspective" argument being even
> > valid.  Big shops still use UDP and it has to scale.
> 
> Preface - I too think it is massively stupid to ignore anything but TCP/IPv4, 
> and unwise to ignore IPv6 and so on, but there is a very real reason why one of 
> my email signatures reads:
> 
> "The road to hell is paved with business decisions"
> 
> > Or have they made multicast magically start working with TCP so
> > they can us it to do trades on the NASDAQ?
> 
> No. How many NIC chips can NASDAQ be expected to move? 0.1%? or even 1% of the 
> NIC chip market?
> 
> How many more NIC chips are in places where someone says "You sold me on 
> iSCSI/FCoE/whatnot, why can't I get 'link-rate'  to/from iSCSI storage/whatnot?!"
> 
> The NIC designer is there with his finance guys breathing down his neck shouting 
> "ROI Uber Alles!" and "Your budget is only this many monetary units!"  The 
> system designers at the system vendors are hearing the same things from their 
> own finance guys, have certain schedules, which then has them going to the NIC 
> firms, who want to sell chips to the system guys "You have to be ready to ship 
> by this date and your chip has to sell for no more than this."
> 
> Lather, rinse, repeat a few times and you get compromises on top of compromises.
> 
> Sometimes I think it is a wonder any of it actually works at all...
> 
> rick jones

Though the thread is going in a different direction now, I just wanted
to clarify two things:
- yes, the 57710 and 57711 only handle the IP (src+dst) for UDP toeplitz
hash. We all agree that it is much better to address the UDP ports as
well, but I think Rick Jones explained the process very well - thank you
Rick. Just to add one more (lame) excuse: the HW was designed before new
NAPI was introduced and it complies with the requirements from Redmond
- the next generation (57712) which we already sample does (finally)
support it. We are working on a patch series to enhance the bnx2x to
support this device now.

Eilon

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

[Eric Dumazet]

Le mardi 27 avril 2010 à 21:31 +0300, Eilon Greenstein a écrit :

> Though the thread is going in a different direction now, I just wanted
> to clarify two things:
> - yes, the 57710 and 57711 only handle the IP (src+dst) for UDP toeplitz
> hash. We all agree that it is much better to address the UDP ports as
> well, but I think Rick Jones explained the process very well - thank you
> Rick. Just to add one more (lame) excuse: the HW was designed before new
> NAPI was introduced and it complies with the requirements from Redmond
> - the next generation (57712) which we already sample does (finally)
> support it. We are working on a patch series to enhance the bnx2x to
> support this device now.
> 

Thanks Eilon !

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

[Stephen Hemminger]

On Mon, 26 Apr 2010 13:40:51 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 26 Apr 2010 13:19:31 -0700
> 
> > As a networking guy I can see why it seems baffling, but stepping out
> > of myself and thinking like the customers with whom I've interacted
> > over the years, it is not baffling at all.
> 
> <sarcasm>
> And hey nobody is using SCTP either, that's right, nobody...
> </sarcasm>
> 
> Look, don't try to defend this abomination of a situation with some
> "customers only use TCP" argument.  It only makes the situation look
> even more absurd.  
> 
> Furthermore, people test system scalability using tools like pktgen,
> which surprise surprise generates streams of UDP packets.  Most
> hardware based scalability testers spew UDP too.
> 
> Everything in the world points to "this toeplitz hash situation is
> stupid an inexcusable."
> 
> If UDP isn't used by anyone, then you tell me why the checksum engines
> of all of these chips can handle them just fine.  Maybe the guy who
> works on the checksum logic blocks doesn't talk to the guy who works
> on the hashing ones?  Maybe the checksum guy can find the ports in a
> UDP packet, but the hashing dude can't locate them?
> 
> What the heck do you think people use for various forms of media
> streaming?  They often use UDP and it has to scale, and they'd like to
> move to DCCP at some point too which is another argument for a fully
> protocol agnostic hash.
> 
> Why do you think Eric Dumazet gives a crap about UDP scalability and
> is constantly testing it?  What about VOIP?  H.323, RTP, etc.?
> 
> Some of these cards can even statelessly offload UDP fragmentation
> too, in silicon, not even in firmware.  What's their excuse for
> screwing up the hash?
> 
> Look, this is a complete joke from every angle, at least admit that
> fact.

I think it is fair to blame Microsoft for this as well. The vendors
follow what Msft tells them to do with NDIS spec. It looks like
IPV6 didn't make it in until the NDIS6.2 (Win7) spec.

-- 

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

[jamal]

On Mon, 2010-04-26 at 13:40 -0700, David Miller wrote:

> Why do you think Eric Dumazet gives a crap about UDP scalability and
> is constantly testing it?  What about VOIP?  H.323, RTP, etc.?

This is big of course. All the SIP/RTP stuff is largely UDP. Most P2P
control is via UDP. I think i did read somewhere on measurement taken
in backbones showing consistent rise of UDP:TCP ratio (need to search
my bookmarks). 
It borders on lunacy to assume TCP only. Time to get an open source NIC
out there and ignore these big players? ;->

cheers,
jamal

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

[jamal]

On Mon, 2010-04-26 at 17:11 -0400, jamal wrote:
> I think i did read somewhere on measurement taken
> in backbones showing consistent rise of UDP:TCP ratio (need to search
> my bookmarks). 

This one:
http://www.caida.org/research/traffic-analysis/tcpudpratio/

just read the conclusion.

cheers,
jamal

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

[Brian Bloniarz]

Rick Jones wrote:
> By and large, customers do not do anything "substantial" with UDP.  NFS
> went to TCP mounts 99 times out of 10 many years ago, leaving DNS as
> about the only thing left*. So, customers will not be chomping at the
> bit for improved UDP scalability/performance.  They would though, be
> jumping up and down demanding iSCSI performance and by implication all
> that comes along for the ride.

I work for a finance firm, and I've met a lot of guys who are
chomping at the bit for improved UDP performance. It's a
huge huge deal in our field. I'm nothing more than one customer
but I have a pretty negative reaction to this sentiment.

I'm also one of the people who occasionally bugs Eric Dumazet
about it :)

One saving grace for the financial multicast case: I think it's
pretty typical for the dest IP to be different for each flow
(different multicast groups). I confirmed on my multi-queue
bnx2 NIC (BCM5709) that those are spread out across the queues,
presumably the hash is similar.

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

[Brian Bloniarz]

David Miller wrote:
> How damn hard is it to add two 16-bit ports to the hash regardless of
> protocol?
>   
Come to think of it, for UDP the hash must ignore
the srcport and srcaddr, because a single bound
socket is going to wildcard both those fields.

So the best we can hope for is for the hash to
include destport and destaddr? From looking at
my BCM5709's multiq behavior, I think broadcom's
hash includes the destaddr but not the destport.

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

[Eric Dumazet]

Le mardi 27 avril 2010 à 09:37 -0400, Brian Bloniarz a écrit :
> David Miller wrote:
> > How damn hard is it to add two 16-bit ports to the hash regardless of
> > protocol?
> >   
> Come to think of it, for UDP the hash must ignore
> the srcport and srcaddr, because a single bound
> socket is going to wildcard both those fields.
> 

For your application maybe ;)

Here, I have thousand of RTP flows to big mediagateways, so the
(srcaddr, dstaddr) is shared by all these flows.

Tom Herbert also wants a threaded DNS server.

For UDP, we could have a bitmap (system level ?) to say if a particular
destination port wants multi-cpu (RPS) spreading or not, even if the NIC
decided to use a single queue to submit frames to the host (ie mask the
src_addr and src_port). In this case, RFS on non conected UDP sockets
could be activated as well.

Or just a global sysctl to be able to mask the src_addr and/or src_port
in our software rxhash.




> So the best we can hope for is for the hash to
> include destport and destaddr? From looking at
> my BCM5709's multiq behavior, I think broadcom's
> hash includes the destaddr but not the destport.


Toepliz hash for UDP is a hash(src_addr, dst_addr)

Both src port and dst port are ignored.

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

[Brian Bloniarz]

Eric Dumazet wrote:
> Le mardi 27 avril 2010 à 09:37 -0400, Brian Bloniarz a écrit :
>> David Miller wrote:
>>> How damn hard is it to add two 16-bit ports to the hash regardless of
>>> protocol?
>>>   
>> Come to think of it, for UDP the hash must ignore
>> the srcport and srcaddr, because a single bound
>> socket is going to wildcard both those fields.
>>
> 
> For your application maybe ;)
> 
> Here, I have thousand of RTP flows to big mediagateways, so the
> (srcaddr, dstaddr) is shared by all these flows.
> 
> Tom Herbert also wants a threaded DNS server.
> 
> For UDP, we could have a bitmap (system level ?) to say if a particular
> destination port wants multi-cpu (RPS) spreading or not, even if the NIC
> decided to use a single queue to submit frames to the host (ie mask the
> src_addr and src_port). In this case, RFS on non conected UDP sockets
> could be activated as well.
> 
> Or just a global sysctl to be able to mask the src_addr and/or src_port
> in our software rxhash.

This is all good to know. Here are 3 other alternatives/suggestions:

1) Leave things as they are, if it really hurts that badly
for our workload we can just disable RPS entirely.
2) Add a global sysctl to disable RPS for datagram-based protocols.
3) Pluggable SW rxhashes :)

I haven't benchmarked anything for our workloads yet, so
I can't say for sure it's even a big issue.

Has anyone benchmarked RPS + single-threaded DNS servers? It'd
be a pessimization in that case, right? Even the multi-threaded
DNS server wouldn't be workable unless there was something like
the soreuseport patch you & Tom had been been discussing.

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

[David Miller]

From: Brian Bloniarz <bmb@athenacr.com>
Date: Tue, 27 Apr 2010 09:37:11 -0400

> David Miller wrote:
>> How damn hard is it to add two 16-bit ports to the hash regardless of
>> protocol?
>>   
> Come to think of it, for UDP the hash must ignore
> the srcport and srcaddr, because a single bound
> socket is going to wildcard both those fields.

For load distribution we don't care if the local socket is wildcard
bounded on source.

It's going to be fully specified in the packet, and that's enough.

Sure, for full RFS some amends might be necessary in this area, but
for RPS and adapter based hw steering, using all of the ports is
entirely sufficient.

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

[Brian Bloniarz]

David Miller wrote:
> From: Brian Bloniarz <bmb@athenacr.com>
> Date: Tue, 27 Apr 2010 09:37:11 -0400
> 
>> David Miller wrote:
>>> How damn hard is it to add two 16-bit ports to the hash regardless of
>>> protocol?
>>>   
>> Come to think of it, for UDP the hash must ignore
>> the srcport and srcaddr, because a single bound
>> socket is going to wildcard both those fields.
> 
> For load distribution we don't care if the local socket is wildcard
> bounded on source.
> 
> It's going to be fully specified in the packet, and that's enough.

Maybe I'm misunderstanding... won't it distribute the
packet handling load to multiple cores, but then all
those cores will contend trying to deliver those packets
to the single socket?

I was assuming that this'd be a net loss over just doing
all the protocol handling on a single core. I haven't
done any benchmarks yet.

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

[David Miller]

From: Brian Bloniarz <bmb@athenacr.com>
Date: Tue, 27 Apr 2010 13:02:08 -0400

> Maybe I'm misunderstanding... won't it distribute the
> packet handling load to multiple cores, but then all
> those cores will contend trying to deliver those packets
> to the single socket?
> 
> I was assuming that this'd be a net loss over just doing
> all the protocol handling on a single core. I haven't
> done any benchmarks yet.

Whether it's a new loss depends upon the application.

Also, on the non-application side f.e. a router or firewall, this is
exactly the behavior you want.

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

[Eric Dumazet]

Le mardi 27 avril 2010 à 09:51 -0700, David Miller a écrit :
> From: Brian Bloniarz <bmb@athenacr.com>
> Date: Tue, 27 Apr 2010 09:37:11 -0400
> 
> > David Miller wrote:
> >> How damn hard is it to add two 16-bit ports to the hash regardless of
> >> protocol?
> >>   
> > Come to think of it, for UDP the hash must ignore
> > the srcport and srcaddr, because a single bound
> > socket is going to wildcard both those fields.
> 
> For load distribution we don't care if the local socket is wildcard
> bounded on source.
> 
> It's going to be fully specified in the packet, and that's enough.
> 
> Sure, for full RFS some amends might be necessary in this area, but
> for RPS and adapter based hw steering, using all of the ports is
> entirely sufficient.

Well well well...

I was doing some pktgen tests, with :

 pgset "src_min 192.168.0.10"
 pgset "src_max 192.168.0.110"
 pgset "dst_min 192.168.0.2"
 pgset "dst_max 192.168.0.2"
 pgset "udp_dst_min 4000"
 pgset "udp_dst_max 4000"

So I simulate 100 remote IPS bombarding a single port on target machine.
pktgen injects about 930.000 pps

sofirq of my target received on cpu0, and RPS spread packets to 7 other
cpus.

And my receiver is stuck (he can read about 50 pps !!!)


As soon as I disable rps, my receiver can catch 850.000 pps


RPS OFF: perf top of cpu 0

------------------------------------------------------------------------------------------------------------------------------
   PerfTop:    1001 irqs/sec  kernel:100.0% [1000Hz cycles],  (all, cpu: 0)
------------------------------------------------------------------------------------------------------------------------------

             samples  pcnt function               DSO
             _______ _____ ______________________ _______

              385.00 10.2% __udp4_lib_lookup      vmlinux
              322.00  8.5% ip_route_input         vmlinux
              312.00  8.3% sock_queue_rcv_skb     vmlinux
              262.00  6.9% do_raw_spin_lock       vmlinux
              251.00  6.6% __alloc_skb            vmlinux
              239.00  6.3% sock_put               vmlinux
              207.00  5.5% eth_type_trans         vmlinux
              202.00  5.4% __slab_alloc           vmlinux
              159.00  4.2% __kmalloc_track_caller vmlinux
              149.00  3.9% __sk_mem_schedule      vmlinux
              125.00  3.3% kmem_cache_alloc       vmlinux
              116.00  3.1% ipt_do_table           vmlinux
              115.00  3.0% do_raw_read_lock       vmlinux
               71.00  1.9% tg3_poll_work          vmlinux
               65.00  1.7% __netdev_alloc_skb     vmlinux
               64.00  1.7% skb_pull               vmlinux
               58.00  1.5% ip_rcv                 vmlinux
               58.00  1.5% __slab_free            vmlinux
               53.00  1.4% udp_queue_rcv_skb      vmlinux
               47.00  1.2% nf_iterate             vmlinux
               44.00  1.2% __netif_receive_skb    vmlinux
               29.00  0.8% sock_def_readable      vmlinux
               28.00  0.7% do_raw_spin_unlock     vmlinux
               26.00  0.7% kfree                  vmlinux
               25.00  0.7% __udp4_lib_rcv         vmlinux
               24.00  0.6% ip_rcv_finish          vmlinux
               24.00  0.6% __list_add             vmlinux


RPS, on, a perf top of a slave CPU :

------------------------------------------------------------------------------------------------------------------------------
   PerfTop:    1000 irqs/sec  kernel:100.0% [1000Hz cycles],  (all, cpu: 1)
------------------------------------------------------------------------------------------------------------------------------

             samples  pcnt function            DSO
             _______ _____ ___________________ _______

             2411.00 62.0% do_raw_spin_lock    vmlinux
              690.00 17.7% delay_tsc           vmlinux
              234.00  6.0% __udp4_lib_lookup   vmlinux
              174.00  4.5% sock_put            vmlinux
               72.00  1.9% ip_rcv              vmlinux
               51.00  1.3% __netif_receive_skb vmlinux
               43.00  1.1% do_raw_spin_unlock  vmlinux
               39.00  1.0% __delay             vmlinux
               38.00  1.0% sock_queue_rcv_skb  vmlinux
               36.00  0.9% udp_queue_rcv_skb   vmlinux
               31.00  0.8% ip_route_input      vmlinux
               15.00  0.4% __slab_free         vmlinux
               12.00  0.3% ipt_do_table        vmlinux
               11.00  0.3% skb_release_data    vmlinux
                7.00  0.2% kfree               vmlinux
                5.00  0.1% nf_iterate          vmlinux

So we have a BIG problem :

All cpus are fighting to get the socket lock,
and very litle progress is done.

Note this problem has nothing to do with RPS, we could have 
it with multiqueue as well.

Oh well...

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Apr 2010 19:13:59 +0200

> So we have a BIG problem :
> 
> All cpus are fighting to get the socket lock,
> and very litle progress is done.
> 
> Note this problem has nothing to do with RPS, we could have 
> it with multiqueue as well.
> 
> Oh well...

Indeed, a huge issue, in that we haven't converted the UDP hash over
to RCU yet :-)

But because of the transient bind nature of UDP there are still a bunch
of cases that won't even cure.

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

[Eric Dumazet]

Le mardi 27 avril 2010 à 10:20 -0700, David Miller a écrit :

> 
> Indeed, a huge issue, in that we haven't converted the UDP hash over
> to RCU yet :-)
> 

I am not sure what you mean, UDP hash _is_ RCU converted ;)

> But because of the transient bind nature of UDP there are still a bunch
> of cases that won't even cure.
> --

We might use the ticket spinlock paradigm to let writers go in parallel
and let the user the socket lock

Instead of having the bh_lock_sock() to protect receive_queue *and*
backlog, writers get a unique slot in a table, that 'user' can handle
later.

Or serialize writers (before they try to bh_lock_sock()) with a
dedicated lock, so that user has 50% chances to get the sock lock,
contending with at most one writer.

[PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[Eric Dumazet]

Le mardi 27 avril 2010 à 19:37 +0200, Eric Dumazet a écrit :

> We might use the ticket spinlock paradigm to let writers go in parallel
> and let the user the socket lock
> 
> Instead of having the bh_lock_sock() to protect receive_queue *and*
> backlog, writers get a unique slot in a table, that 'user' can handle
> later.
> 
> Or serialize writers (before they try to bh_lock_sock()) with a
> dedicated lock, so that user has 50% chances to get the sock lock,
> contending with at most one writer.

Following patch fixes the issue for me, with little performance hit on
fast path.

Under huge stress from a multiqueue/RPS enabled NIC, a single flow udp
receiver can now process ~200.000 pps (instead of ~100 pps before the
patch) on my dev machine.

Thanks !

[PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

Current socket backlog limit is not enough to really stop DDOS attacks,
because user thread spend many time to process a full backlog each
round, and user might crazy spin on socket lock.

We should add backlog size and receive_queue size (aka rmem_alloc) to
pace writers, and let user run without being slow down too much.

Introduce a sk_rcvqueues_full() helper, to avoid taking socket lock in
stress situations.

Under huge stress from a multiqueue/RPS enabled NIC, a single flow udp
receiver can now process ~200.000 pps (instead of ~100 pps before the
patch) on a 8 core machine.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/net/sock.h |   13 +++++++++++--
 net/core/sock.c    |    5 ++++-
 net/ipv4/udp.c     |    4 ++++
 net/ipv6/udp.c     |    8 ++++++++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 86a8ca1..4b0097d 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -255,7 +255,6 @@ struct sock {
 		struct sk_buff *head;
 		struct sk_buff *tail;
 		int len;
-		int limit;
 	} sk_backlog;
 	wait_queue_head_t	*sk_sleep;
 	struct dst_entry	*sk_dst_cache;
@@ -604,10 +603,20 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
 	skb->next = NULL;
 }
 
+/*
+ * Take into account size of receive queue and backlog queue
+ */
+static inline bool sk_rcvqueues_full(const struct sock *sk, const struct sk_buff *skb)
+{
+	unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
+
+	return qsize + skb->truesize > sk->sk_rcvbuf;
+}
+
 /* The per-socket spinlock must be held here. */
 static inline __must_check int sk_add_backlog(struct sock *sk, struct sk_buff *skb)
 {
-	if (sk->sk_backlog.len >= max(sk->sk_backlog.limit, sk->sk_rcvbuf << 1))
+	if (sk_rcvqueues_full(sk, skb))
 		return -ENOBUFS;
 
 	__sk_add_backlog(sk, skb);
diff --git a/net/core/sock.c b/net/core/sock.c
index 58ebd14..5104175 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -327,6 +327,10 @@ int sk_receive_skb(struct sock *sk, struct sk_buff *skb, const int nested)
 
 	skb->dev = NULL;
 
+	if (sk_rcvqueues_full(sk, skb)) {
+		atomic_inc(&sk->sk_drops);
+		goto discard_and_relse;
+	}
 	if (nested)
 		bh_lock_sock_nested(sk);
 	else
@@ -1885,7 +1889,6 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	sk->sk_allocation	=	GFP_KERNEL;
 	sk->sk_rcvbuf		=	sysctl_rmem_default;
 	sk->sk_sndbuf		=	sysctl_wmem_default;
-	sk->sk_backlog.limit	=	sk->sk_rcvbuf << 1;
 	sk->sk_state		=	TCP_CLOSE;
 	sk_set_socket(sk, sock);
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 1e18f9c..776c844 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1372,6 +1372,10 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 			goto drop;
 	}
 
+
+	if (sk_rcvqueues_full(sk, skb))
+		goto drop;
+
 	rc = 0;
 
 	bh_lock_sock(sk);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 2850e35..3ead20a 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -584,6 +584,10 @@ static void flush_stack(struct sock **stack, unsigned int count,
 
 		sk = stack[i];
 		if (skb1) {
+			if (sk_rcvqueues_full(sk, skb)) {
+				kfree_skb(skb1);
+				goto drop;
+			}
 			bh_lock_sock(sk);
 			if (!sock_owned_by_user(sk))
 				udpv6_queue_rcv_skb(sk, skb1);
@@ -759,6 +763,10 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 
 	/* deliver */
 
+	if (sk_rcvqueues_full(sk, skb)) {
+		sock_put(sk);
+		goto discard;
+	}
 	bh_lock_sock(sk);
 	if (!sock_owned_by_user(sk))
 		udpv6_queue_rcv_skb(sk, skb);

Re: [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Apr 2010 22:21:02 +0200

> [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account
> 
> Current socket backlog limit is not enough to really stop DDOS attacks,
> because user thread spend many time to process a full backlog each
> round, and user might crazy spin on socket lock.
> 
> We should add backlog size and receive_queue size (aka rmem_alloc) to
> pace writers, and let user run without being slow down too much.
> 
> Introduce a sk_rcvqueues_full() helper, to avoid taking socket lock in
> stress situations.
> 
> Under huge stress from a multiqueue/RPS enabled NIC, a single flow udp
> receiver can now process ~200.000 pps (instead of ~100 pps before the
> patch) on a 8 core machine.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

This looks great, applied, thanks Eric!

Re: [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 27 Apr 2010 14:43:07 -0700 (PDT)

> This looks great, applied, thanks Eric!

I have to fix this one up, for example SCTP stopped building because it
still refers to the ->limit member you removed.

Re: [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[Eric Dumazet]

Le mardi 27 avril 2010 à 15:11 -0700, David Miller a écrit :
> From: David Miller <davem@davemloft.net>
> Date: Tue, 27 Apr 2010 14:43:07 -0700 (PDT)
> 
> > This looks great, applied, thanks Eric!
> 
> I have to fix this one up, for example SCTP stopped building because it
> still refers to the ->limit member you removed.
> --

Hmm, sorry for this, thanks !

Re: [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[Brian Bloniarz]

Eric Dumazet wrote:
> Le mardi 27 avril 2010 à 19:37 +0200, Eric Dumazet a écrit :
> 
>> We might use the ticket spinlock paradigm to let writers go in parallel
>> and let the user the socket lock
>>
>> Instead of having the bh_lock_sock() to protect receive_queue *and*
>> backlog, writers get a unique slot in a table, that 'user' can handle
>> later.
>>
>> Or serialize writers (before they try to bh_lock_sock()) with a
>> dedicated lock, so that user has 50% chances to get the sock lock,
>> contending with at most one writer.
> 
> Following patch fixes the issue for me, with little performance hit on
> fast path.
> 
> Under huge stress from a multiqueue/RPS enabled NIC, a single flow udp
> receiver can now process ~200.000 pps (instead of ~100 pps before the
> patch) on my dev machine.
> 
> Thanks !
> 
> [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account
> 
> Current socket backlog limit is not enough to really stop DDOS attacks,
> because user thread spend many time to process a full backlog each
> round, and user might crazy spin on socket lock.
> 
> We should add backlog size and receive_queue size (aka rmem_alloc) to
> pace writers, and let user run without being slow down too much.
> 
> Introduce a sk_rcvqueues_full() helper, to avoid taking socket lock in
> stress situations.
> 
> Under huge stress from a multiqueue/RPS enabled NIC, a single flow udp
> receiver can now process ~200.000 pps (instead of ~100 pps before the
> patch) on a 8 core machine.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Wow that was awesome.

> ---
>  include/net/sock.h |   13 +++++++++++--
>  net/core/sock.c    |    5 ++++-
>  net/ipv4/udp.c     |    4 ++++
>  net/ipv6/udp.c     |    8 ++++++++
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 86a8ca1..4b0097d 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -255,7 +255,6 @@ struct sock {
>  		struct sk_buff *head;
>  		struct sk_buff *tail;
>  		int len;
> -		int limit;
>  	} sk_backlog;
>  	wait_queue_head_t	*sk_sleep;
>  	struct dst_entry	*sk_dst_cache;
> @@ -604,10 +603,20 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
>  	skb->next = NULL;
>  }
>  
> +/*
> + * Take into account size of receive queue and backlog queue
> + */
> +static inline bool sk_rcvqueues_full(const struct sock *sk, const struct sk_buff *skb)
> +{
> +	unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
> +
> +	return qsize + skb->truesize > sk->sk_rcvbuf;
> +}
> +

Reading sk_backlog.len without the socket lock held seems to
contradict the comment in sock.h:
  *	@sk_backlog: always used with the per-socket spinlock held
  ...
struct sock {

        ...
	/*
	 * The backlog queue is special, it is always used with
	 * the per-socket spinlock held and requires low latency
	 * access. Therefore we special case it's implementation.
	 */
	struct { ... } sk_backlog;

Is this just a doc mismatch or does sk_backlog.len need to use
atomic_read & atomic_set?

>  /* The per-socket spinlock must be held here. */
>  static inline __must_check int sk_add_backlog(struct sock *sk, struct sk_buff *skb)
>  {
> -	if (sk->sk_backlog.len >= max(sk->sk_backlog.limit, sk->sk_rcvbuf << 1))
> +	if (sk_rcvqueues_full(sk, skb))
>  		return -ENOBUFS;
>  
>  	__sk_add_backlog(sk, skb);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 58ebd14..5104175 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -327,6 +327,10 @@ int sk_receive_skb(struct sock *sk, struct sk_buff *skb, const int nested)
>  
>  	skb->dev = NULL;
>  
> +	if (sk_rcvqueues_full(sk, skb)) {
> +		atomic_inc(&sk->sk_drops);
> +		goto discard_and_relse;
> +	}
>  	if (nested)
>  		bh_lock_sock_nested(sk);
>  	else
> @@ -1885,7 +1889,6 @@ void sock_init_data(struct socket *sock, struct sock *sk)
>  	sk->sk_allocation	=	GFP_KERNEL;
>  	sk->sk_rcvbuf		=	sysctl_rmem_default;
>  	sk->sk_sndbuf		=	sysctl_wmem_default;
> -	sk->sk_backlog.limit	=	sk->sk_rcvbuf << 1;
>  	sk->sk_state		=	TCP_CLOSE;
>  	sk_set_socket(sk, sock);
>  
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 1e18f9c..776c844 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1372,6 +1372,10 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  			goto drop;
>  	}
>  
> +
> +	if (sk_rcvqueues_full(sk, skb))
> +		goto drop;
> +
>  	rc = 0;
>  
>  	bh_lock_sock(sk);
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 2850e35..3ead20a 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -584,6 +584,10 @@ static void flush_stack(struct sock **stack, unsigned int count,
>  
>  		sk = stack[i];
>  		if (skb1) {
> +			if (sk_rcvqueues_full(sk, skb)) {
> +				kfree_skb(skb1);
> +				goto drop;
> +			}
>  			bh_lock_sock(sk);
>  			if (!sock_owned_by_user(sk))
>  				udpv6_queue_rcv_skb(sk, skb1);
> @@ -759,6 +763,10 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  
>  	/* deliver */
>  
> +	if (sk_rcvqueues_full(sk, skb)) {
> +		sock_put(sk);
> +		goto discard;
> +	}
>  	bh_lock_sock(sk);
>  	if (!sock_owned_by_user(sk))
>  		udpv6_queue_rcv_skb(sk, skb);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next-2.6] net: sk_add_backlog() take rmem_alloc into account

[Eric Dumazet]

Le mercredi 28 avril 2010 à 11:41 -0400, Brian Bloniarz a écrit :

> >  
> > +/*
> > + * Take into account size of receive queue and backlog queue
> > + */
> > +static inline bool sk_rcvqueues_full(const struct sock *sk, const struct sk_buff *skb)
> > +{
> > +	unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
> > +
> > +	return qsize + skb->truesize > sk->sk_rcvbuf;
> > +}
> > +
> 
> Reading sk_backlog.len without the socket lock held seems to
> contradict the comment in sock.h:
>   *	@sk_backlog: always used with the per-socket spinlock held
>   ...
> struct sock {
> 
>         ...
> 	/*
> 	 * The backlog queue is special, it is always used with
> 	 * the per-socket spinlock held and requires low latency
> 	 * access. Therefore we special case it's implementation.
> 	 */
> 	struct { ... } sk_backlog;
> 
> Is this just a doc mismatch or does sk_backlog.len need to use
> atomic_read & atomic_set?
> 

I'll submit a doc cleanup, and will avoid this 32bit hole in a reorg of
struct sock layout.

We read 'sk_backlog.len'  without lock to have a hint. We could have a
false positive only when queue is full, so this is not a big deal.

Then, after locking, we call sk_rcvqueues_full() once again.

Thanks for reviewing !

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

[Tom Herbert]

> So we have a BIG problem :
>
> All cpus are fighting to get the socket lock,
> and very litle progress is done.
>
> Note this problem has nothing to do with RPS, we could have
> it with multiqueue as well.
>

This is the problem that we are addressing with so_reuseport!

> Oh well...
>
>
>
>

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

[Eric Dumazet]

Le mardi 27 avril 2010 à 10:31 -0700, Tom Herbert a écrit :

> This is the problem that we are addressing with so_reuseport!

How standard applications are protected against a DDOS ?

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

[Vladislav Zolotarov]

Tom, could u, pls., explain what did u mean by taking the (RSS) flags configuration out of RSS "if"? To recall "if(is_multi(bp))" is true iff RSS is enabled.

Thanks,
vlad

> @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> *bp)
>  	u32 offset;
>  	u16 max_agg_size;
> 
> -	if (is_multi(bp)) {
> -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> +	tstorm_config.config_flags = RSS_FLAGS(bp);
> +
> +	if (is_multi(bp))
>  		tstorm_config.rss_result_mask = MULTI_MASK;
> -	}
> 
>  	/* Enable TPA if needed */
>  	if (bp->flags & TPA_ENABLE_FLAG)


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Tom Herbert
> Sent: Friday, April 23, 2010 8:54 AM
> To: davem@davemloft.net; netdev@vger.kernel.org
> Subject: [PATCH] bnx2x: add support for receive hashing
> 
> Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
> for use in skb->rxhash.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> index 0819530..8bd2368 100644
> --- a/drivers/net/bnx2x.h
> +++ b/drivers/net/bnx2x.h
> @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg,
> u32 expected, int ms,
>  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
>  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
> 
> -#define MULTI_FLAGS(bp) \
> +#define RSS_FLAGS(bp) \
>  		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
>  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
>  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 0c6dba2..613f727 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int
> budget)
>  		struct sw_rx_bd *rx_buf = NULL;
>  		struct sk_buff *skb;
>  		union eth_rx_cqe *cqe;
> -		u8 cqe_fp_flags;
> +		u8 cqe_fp_flags, cqe_fp_status_flags;
>  		u16 len, pad;
> 
>  		comp_ring_cons = RCQ_BD(sw_comp_cons);
> @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp, int
> budget)
> 
>  		cqe = &fp->rx_comp_ring[comp_ring_cons];
>  		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
> +		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
> 
>  		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
>  		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
> @@ -1727,6 +1728,12 @@ reuse_rx:
> 
>  			skb->protocol = eth_type_trans(skb, bp->dev);
> 
> +			if ((bp->dev->features & ETH_FLAG_RXHASH) &&
> +			    (cqe_fp_status_flags &
> +			     ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
> +				skb->rxhash = le32_to_cpu(
> +				    cqe->fast_path_cqe.rss_hash_result);
> +
>  			skb->ip_summed = CHECKSUM_NONE;
>  			if (bp->rx_csum) {
>  				if (likely(BNX2X_RX_CSUM_OK(cqe)))
> @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> *bp)
>  	u32 offset;
>  	u16 max_agg_size;
> 
> -	if (is_multi(bp)) {
> -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> +	tstorm_config.config_flags = RSS_FLAGS(bp);
> +
> +	if (is_multi(bp))
>  		tstorm_config.rss_result_mask = MULTI_MASK;
> -	}
> 
>  	/* Enable TPA if needed */
>  	if (bp->flags & TPA_ENABLE_FLAG)
> @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
>  	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
> 
>  	REG_WR(bp, SRC_REG_SOFT_RST, 1);
> -	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> -		REG_WR(bp, i, 0xc0cac01a);
> -		/* TODO: replace with something meaningful */
> -	}
> +	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> +		REG_WR(bp, i, random32());
>  	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
>  #ifdef BCM_CNIC
>  	REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
> @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev,
> u32 data)
>  		changed = 1;
>  	}
> 
> +	if (data & ETH_FLAG_RXHASH)
> +		dev->features |= NETIF_F_RXHASH;
> +	else
> +		dev->features &= ~NETIF_F_RXHASH;
> +
>  	if (changed && netif_running(dev)) {
>  		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
>  		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vladislav Zolotarov]

Is there any reason not to fill skb->rxhash for LROed packets?

Thanks,
vlad

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Vladislav Zolotarov
> Sent: Sunday, July 04, 2010 7:36 PM
> To: Tom Herbert
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] bnx2x: add support for receive hashing
> 
> Tom, could u, pls., explain what did u mean by taking the (RSS) flags
> configuration out of RSS "if"? To recall "if(is_multi(bp))" is true iff RSS
> is enabled.
> 
> Thanks,
> vlad
> 
> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> > *bp)
> >  	u32 offset;
> >  	u16 max_agg_size;
> >
> > -	if (is_multi(bp)) {
> > -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> > +	tstorm_config.config_flags = RSS_FLAGS(bp);
> > +
> > +	if (is_multi(bp))
> >  		tstorm_config.rss_result_mask = MULTI_MASK;
> > -	}
> >
> >  	/* Enable TPA if needed */
> >  	if (bp->flags & TPA_ENABLE_FLAG)
> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Tom Herbert
> > Sent: Friday, April 23, 2010 8:54 AM
> > To: davem@davemloft.net; netdev@vger.kernel.org
> > Subject: [PATCH] bnx2x: add support for receive hashing
> >
> > Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
> > for use in skb->rxhash.
> >
> > Signed-off-by: Tom Herbert <therbert@google.com>
> > ---
> > diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> > index 0819530..8bd2368 100644
> > --- a/drivers/net/bnx2x.h
> > +++ b/drivers/net/bnx2x.h
> > @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg,
> > u32 expected, int ms,
> >  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
> >  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
> >
> > -#define MULTI_FLAGS(bp) \
> > +#define RSS_FLAGS(bp) \
> >  		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
> >  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
> >  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
> > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> > index 0c6dba2..613f727 100644
> > --- a/drivers/net/bnx2x_main.c
> > +++ b/drivers/net/bnx2x_main.c
> > @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> int
> > budget)
> >  		struct sw_rx_bd *rx_buf = NULL;
> >  		struct sk_buff *skb;
> >  		union eth_rx_cqe *cqe;
> > -		u8 cqe_fp_flags;
> > +		u8 cqe_fp_flags, cqe_fp_status_flags;
> >  		u16 len, pad;
> >
> >  		comp_ring_cons = RCQ_BD(sw_comp_cons);
> > @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> int
> > budget)
> >
> >  		cqe = &fp->rx_comp_ring[comp_ring_cons];
> >  		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
> > +		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
> >
> >  		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
> >  		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
> > @@ -1727,6 +1728,12 @@ reuse_rx:
> >
> >  			skb->protocol = eth_type_trans(skb, bp->dev);
> >
> > +			if ((bp->dev->features & ETH_FLAG_RXHASH) &&
> > +			    (cqe_fp_status_flags &
> > +			     ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
> > +				skb->rxhash = le32_to_cpu(
> > +				    cqe->fast_path_cqe.rss_hash_result);
> > +
> >  			skb->ip_summed = CHECKSUM_NONE;
> >  			if (bp->rx_csum) {
> >  				if (likely(BNX2X_RX_CSUM_OK(cqe)))
> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> > *bp)
> >  	u32 offset;
> >  	u16 max_agg_size;
> >
> > -	if (is_multi(bp)) {
> > -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> > +	tstorm_config.config_flags = RSS_FLAGS(bp);
> > +
> > +	if (is_multi(bp))
> >  		tstorm_config.rss_result_mask = MULTI_MASK;
> > -	}
> >
> >  	/* Enable TPA if needed */
> >  	if (bp->flags & TPA_ENABLE_FLAG)
> > @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
> >  	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
> >
> >  	REG_WR(bp, SRC_REG_SOFT_RST, 1);
> > -	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> > -		REG_WR(bp, i, 0xc0cac01a);
> > -		/* TODO: replace with something meaningful */
> > -	}
> > +	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> > +		REG_WR(bp, i, random32());
> >  	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
> >  #ifdef BCM_CNIC
> >  	REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
> > @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev,
> > u32 data)
> >  		changed = 1;
> >  	}
> >
> > +	if (data & ETH_FLAG_RXHASH)
> > +		dev->features |= NETIF_F_RXHASH;
> > +	else
> > +		dev->features &= ~NETIF_F_RXHASH;
> > +
> >  	if (changed && netif_running(dev)) {
> >  		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> >  		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vladislav Zolotarov]

Let me rephrase: the part of your patch below enables RSS flow in our FW even if there is only one HW queue and I wonder why? 
To refresh:
	1) FW won't provide Toeplitz hash on CQE if RSS is not enabled.
	2) There can be one HW queue in only 2 cases:
		- There is only one CPU in the system. In that case I wonder if u have anything to do with Toeplitz hash on the skb at all.
		- User has explicitly requested 1 HW queue with module parameter (num_queues=1). In that case if u r going to use the RSS hash on the skb means that u r actually going to do SW RSS instead of HW RSS, which sounds strange to me.

So, Herbert, could u, pls., explain, what was your original idea about these code lines?

Thanks,
vlad

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Vladislav Zolotarov
> Sent: Sunday, July 04, 2010 7:36 PM
> To: Tom Herbert
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] bnx2x: add support for receive hashing
> 
> Tom, could u, pls., explain what did u mean by taking the (RSS) flags
> configuration out of RSS "if"? To recall "if(is_multi(bp))" is true iff RSS
> is enabled.
> 
> Thanks,
> vlad
> 
> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> > *bp)
> >  	u32 offset;
> >  	u16 max_agg_size;
> >
> > -	if (is_multi(bp)) {
> > -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> > +	tstorm_config.config_flags = RSS_FLAGS(bp);
> > +
> > +	if (is_multi(bp))
> >  		tstorm_config.rss_result_mask = MULTI_MASK;
> > -	}
> >
> >  	/* Enable TPA if needed */
> >  	if (bp->flags & TPA_ENABLE_FLAG)
> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Tom Herbert
> > Sent: Friday, April 23, 2010 8:54 AM
> > To: davem@davemloft.net; netdev@vger.kernel.org
> > Subject: [PATCH] bnx2x: add support for receive hashing
> >
> > Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
> > for use in skb->rxhash.
> >
> > Signed-off-by: Tom Herbert <therbert@google.com>
> > ---
> > diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> > index 0819530..8bd2368 100644
> > --- a/drivers/net/bnx2x.h
> > +++ b/drivers/net/bnx2x.h
> > @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg,
> > u32 expected, int ms,
> >  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
> >  		AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
> >
> > -#define MULTI_FLAGS(bp) \
> > +#define RSS_FLAGS(bp) \
> >  		(TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
> >  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
> >  		 TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
> > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> > index 0c6dba2..613f727 100644
> > --- a/drivers/net/bnx2x_main.c
> > +++ b/drivers/net/bnx2x_main.c
> > @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> int
> > budget)
> >  		struct sw_rx_bd *rx_buf = NULL;
> >  		struct sk_buff *skb;
> >  		union eth_rx_cqe *cqe;
> > -		u8 cqe_fp_flags;
> > +		u8 cqe_fp_flags, cqe_fp_status_flags;
> >  		u16 len, pad;
> >
> >  		comp_ring_cons = RCQ_BD(sw_comp_cons);
> > @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> int
> > budget)
> >
> >  		cqe = &fp->rx_comp_ring[comp_ring_cons];
> >  		cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
> > +		cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
> >
> >  		DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
> >  		   "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
> > @@ -1727,6 +1728,12 @@ reuse_rx:
> >
> >  			skb->protocol = eth_type_trans(skb, bp->dev);
> >
> > +			if ((bp->dev->features & ETH_FLAG_RXHASH) &&
> > +			    (cqe_fp_status_flags &
> > +			     ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
> > +				skb->rxhash = le32_to_cpu(
> > +				    cqe->fast_path_cqe.rss_hash_result);
> > +
> >  			skb->ip_summed = CHECKSUM_NONE;
> >  			if (bp->rx_csum) {
> >  				if (likely(BNX2X_RX_CSUM_OK(cqe)))
> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
> > *bp)
> >  	u32 offset;
> >  	u16 max_agg_size;
> >
> > -	if (is_multi(bp)) {
> > -		tstorm_config.config_flags = MULTI_FLAGS(bp);
> > +	tstorm_config.config_flags = RSS_FLAGS(bp);
> > +
> > +	if (is_multi(bp))
> >  		tstorm_config.rss_result_mask = MULTI_MASK;
> > -	}
> >
> >  	/* Enable TPA if needed */
> >  	if (bp->flags & TPA_ENABLE_FLAG)
> > @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
> >  	bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
> >
> >  	REG_WR(bp, SRC_REG_SOFT_RST, 1);
> > -	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> > -		REG_WR(bp, i, 0xc0cac01a);
> > -		/* TODO: replace with something meaningful */
> > -	}
> > +	for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> > +		REG_WR(bp, i, random32());
> >  	bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
> >  #ifdef BCM_CNIC
> >  	REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
> > @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev,
> > u32 data)
> >  		changed = 1;
> >  	}
> >
> > +	if (data & ETH_FLAG_RXHASH)
> > +		dev->features |= NETIF_F_RXHASH;
> > +	else
> > +		dev->features &= ~NETIF_F_RXHASH;
> > +
> >  	if (changed && netif_running(dev)) {
> >  		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> >  		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tom Herbert]

On Tue, Jul 6, 2010 at 12:16 AM, Vladislav Zolotarov <vladz@broadcom.com> wrote:
> Let me rephrase: the part of your patch below enables RSS flow in our FW even if there is only one HW queue and I wonder why?
> To refresh:
>        1) FW won't provide Toeplitz hash on CQE if RSS is not enabled.
>        2) There can be one HW queue in only 2 cases:
>                - There is only one CPU in the system. In that case I wonder if u have anything to do with Toeplitz hash on the skb at all.
>                - User has explicitly requested 1 HW queue with module parameter (num_queues=1). In that case if u r going to use the RSS hash on the skb means that u r actually going to do SW RSS instead of HW RSS, which sounds strange to me.

It might not be so strange.  Previously, we has hit a firmware bug in
bnx2x that was make multi-queue not perform well under some loads, so
we had disabled it for a while... it is a valid configuration we have
used.

>
> So, Herbert, could u, pls., explain, what was your original idea about these code lines?
>

It is to enable the device to provide the RSS hash in RX descriptor.
The hash severs two purposes now, it's used internally in the device
to perform RSS table lookup and also value in RX descriptor.  The
latter does not require multi-queue.  Strictly speaking, on a single
processor system without multqueue, it would be true that enabling the
RX hash on bnx2x is currently superfluous (notwithstanding some other
use of the hash might be implemented).

> Thanks,
> vlad
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>> Behalf Of Vladislav Zolotarov
>> Sent: Sunday, July 04, 2010 7:36 PM
>> To: Tom Herbert
>> Cc: netdev@vger.kernel.org
>> Subject: RE: [PATCH] bnx2x: add support for receive hashing
>>
>> Tom, could u, pls., explain what did u mean by taking the (RSS) flags
>> configuration out of RSS "if"? To recall "if(is_multi(bp))" is true iff RSS
>> is enabled.
>>
>> Thanks,
>> vlad
>>
>> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
>> > *bp)
>> >     u32 offset;
>> >     u16 max_agg_size;
>> >
>> > -   if (is_multi(bp)) {
>> > -           tstorm_config.config_flags = MULTI_FLAGS(bp);
>> > +   tstorm_config.config_flags = RSS_FLAGS(bp);
>> > +
>> > +   if (is_multi(bp))
>> >             tstorm_config.rss_result_mask = MULTI_MASK;
>> > -   }
>> >
>> >     /* Enable TPA if needed */
>> >     if (bp->flags & TPA_ENABLE_FLAG)
>>
>>
>> > -----Original Message-----
>> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
>> > Behalf Of Tom Herbert
>> > Sent: Friday, April 23, 2010 8:54 AM
>> > To: davem@davemloft.net; netdev@vger.kernel.org
>> > Subject: [PATCH] bnx2x: add support for receive hashing
>> >
>> > Add support to bnx2x to extract Toeplitz hash out of the receive descriptor
>> > for use in skb->rxhash.
>> >
>> > Signed-off-by: Tom Herbert <therbert@google.com>
>> > ---
>> > diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
>> > index 0819530..8bd2368 100644
>> > --- a/drivers/net/bnx2x.h
>> > +++ b/drivers/net/bnx2x.h
>> > @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32 reg,
>> > u32 expected, int ms,
>> >             AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
>> >             AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
>> >
>> > -#define MULTI_FLAGS(bp) \
>> > +#define RSS_FLAGS(bp) \
>> >             (TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
>> >              TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY | \
>> >              TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
>> > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
>> > index 0c6dba2..613f727 100644
>> > --- a/drivers/net/bnx2x_main.c
>> > +++ b/drivers/net/bnx2x_main.c
>> > @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
>> int
>> > budget)
>> >             struct sw_rx_bd *rx_buf = NULL;
>> >             struct sk_buff *skb;
>> >             union eth_rx_cqe *cqe;
>> > -           u8 cqe_fp_flags;
>> > +           u8 cqe_fp_flags, cqe_fp_status_flags;
>> >             u16 len, pad;
>> >
>> >             comp_ring_cons = RCQ_BD(sw_comp_cons);
>> > @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
>> int
>> > budget)
>> >
>> >             cqe = &fp->rx_comp_ring[comp_ring_cons];
>> >             cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
>> > +           cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
>> >
>> >             DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
>> >                "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
>> > @@ -1727,6 +1728,12 @@ reuse_rx:
>> >
>> >                     skb->protocol = eth_type_trans(skb, bp->dev);
>> >
>> > +                   if ((bp->dev->features & ETH_FLAG_RXHASH) &&
>> > +                       (cqe_fp_status_flags &
>> > +                        ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
>> > +                           skb->rxhash = le32_to_cpu(
>> > +                               cqe->fast_path_cqe.rss_hash_result);
>> > +
>> >                     skb->ip_summed = CHECKSUM_NONE;
>> >                     if (bp->rx_csum) {
>> >                             if (likely(BNX2X_RX_CSUM_OK(cqe)))
>> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct bnx2x
>> > *bp)
>> >     u32 offset;
>> >     u16 max_agg_size;
>> >
>> > -   if (is_multi(bp)) {
>> > -           tstorm_config.config_flags = MULTI_FLAGS(bp);
>> > +   tstorm_config.config_flags = RSS_FLAGS(bp);
>> > +
>> > +   if (is_multi(bp))
>> >             tstorm_config.rss_result_mask = MULTI_MASK;
>> > -   }
>> >
>> >     /* Enable TPA if needed */
>> >     if (bp->flags & TPA_ENABLE_FLAG)
>> > @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
>> >     bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
>> >
>> >     REG_WR(bp, SRC_REG_SOFT_RST, 1);
>> > -   for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
>> > -           REG_WR(bp, i, 0xc0cac01a);
>> > -           /* TODO: replace with something meaningful */
>> > -   }
>> > +   for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
>> > +           REG_WR(bp, i, random32());
>> >     bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
>> >  #ifdef BCM_CNIC
>> >     REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
>> > @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device *dev,
>> > u32 data)
>> >             changed = 1;
>> >     }
>> >
>> > +   if (data & ETH_FLAG_RXHASH)
>> > +           dev->features |= NETIF_F_RXHASH;
>> > +   else
>> > +           dev->features &= ~NETIF_F_RXHASH;
>> > +
>> >     if (changed && netif_running(dev)) {
>> >             bnx2x_nic_unload(bp, UNLOAD_NORMAL);
>> >             rc = bnx2x_nic_load(bp, LOAD_NORMAL);
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe netdev" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

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

[Vladislav Zolotarov]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Tom Herbert
> Sent: Wednesday, July 07, 2010 10:18 PM
> To: Vladislav Zolotarov
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bnx2x: add support for receive hashing
> 
> On Tue, Jul 6, 2010 at 12:16 AM, Vladislav Zolotarov <vladz@broadcom.com>
> wrote:
> > Let me rephrase: the part of your patch below enables RSS flow in our FW
> even if there is only one HW queue and I wonder why?
> > To refresh:
> >        1) FW won't provide Toeplitz hash on CQE if RSS is not enabled.
> >        2) There can be one HW queue in only 2 cases:
> >                - There is only one CPU in the system. In that case I wonder
> if u have anything to do with Toeplitz hash on the skb at all.
> >                - User has explicitly requested 1 HW queue with module
> parameter (num_queues=1). In that case if u r going to use the RSS hash on
> the skb means that u r actually going to do SW RSS instead of HW RSS, which
> sounds strange to me.
> 
> It might not be so strange.  Previously, we has hit a firmware bug in
> bnx2x that was make multi-queue not perform well under some loads, so
> we had disabled it for a while... it is a valid configuration we have
> used.

Tom, according to our FW guys there are no known RSS issues since FW 5.0.x versions. Currently upstream has 5.2.13 FW so, if u know about any issue with the current upstream bnx2x FW, pls., let us know ASAP. Pls., include the relevant performance data. We would also like to know if u see that the FW bug u've mentioned has been resolved in the current upstream bnx2x FW so that we could remove these lines unless u have any other reason to leave them.

Thanks,
vlad

> 
> >
> > So, Herbert, could u, pls., explain, what was your original idea about
> these code lines?
> >
> 
> It is to enable the device to provide the RSS hash in RX descriptor.
> The hash severs two purposes now, it's used internally in the device
> to perform RSS table lookup and also value in RX descriptor.  The
> latter does not require multi-queue.  Strictly speaking, on a single
> processor system without multqueue, it would be true that enabling the
> RX hash on bnx2x is currently superfluous (notwithstanding some other
> use of the hash might be implemented).
> 
> > Thanks,
> > vlad
> >
> >> -----Original Message-----
> >> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On
> >> Behalf Of Vladislav Zolotarov
> >> Sent: Sunday, July 04, 2010 7:36 PM
> >> To: Tom Herbert
> >> Cc: netdev@vger.kernel.org
> >> Subject: RE: [PATCH] bnx2x: add support for receive hashing
> >>
> >> Tom, could u, pls., explain what did u mean by taking the (RSS) flags
> >> configuration out of RSS "if"? To recall "if(is_multi(bp))" is true iff
> RSS
> >> is enabled.
> >>
> >> Thanks,
> >> vlad
> >>
> >> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct
> bnx2x
> >> > *bp)
> >> >     u32 offset;
> >> >     u16 max_agg_size;
> >> >
> >> > -   if (is_multi(bp)) {
> >> > -           tstorm_config.config_flags = MULTI_FLAGS(bp);
> >> > +   tstorm_config.config_flags = RSS_FLAGS(bp);
> >> > +
> >> > +   if (is_multi(bp))
> >> >             tstorm_config.rss_result_mask = MULTI_MASK;
> >> > -   }
> >> >
> >> >     /* Enable TPA if needed */
> >> >     if (bp->flags & TPA_ENABLE_FLAG)
> >>
> >>
> >> > -----Original Message-----
> >> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On
> >> > Behalf Of Tom Herbert
> >> > Sent: Friday, April 23, 2010 8:54 AM
> >> > To: davem@davemloft.net; netdev@vger.kernel.org
> >> > Subject: [PATCH] bnx2x: add support for receive hashing
> >> >
> >> > Add support to bnx2x to extract Toeplitz hash out of the receive
> descriptor
> >> > for use in skb->rxhash.
> >> >
> >> > Signed-off-by: Tom Herbert <therbert@google.com>
> >> > ---
> >> > diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> >> > index 0819530..8bd2368 100644
> >> > --- a/drivers/net/bnx2x.h
> >> > +++ b/drivers/net/bnx2x.h
> >> > @@ -1330,7 +1330,7 @@ static inline u32 reg_poll(struct bnx2x *bp, u32
> reg,
> >> > u32 expected, int ms,
> >> >             AEU_INPUTS_ATTN_BITS_MCP_LATCHED_UMP_TX_PARITY | \
> >> >             AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY)
> >> >
> >> > -#define MULTI_FLAGS(bp) \
> >> > +#define RSS_FLAGS(bp) \
> >> >             (TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY | \
> >> >              TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_TCP_CAPABILITY |
> \
> >> >              TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_CAPABILITY | \
> >> > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> >> > index 0c6dba2..613f727 100644
> >> > --- a/drivers/net/bnx2x_main.c
> >> > +++ b/drivers/net/bnx2x_main.c
> >> > @@ -1582,7 +1582,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> >> int
> >> > budget)
> >> >             struct sw_rx_bd *rx_buf = NULL;
> >> >             struct sk_buff *skb;
> >> >             union eth_rx_cqe *cqe;
> >> > -           u8 cqe_fp_flags;
> >> > +           u8 cqe_fp_flags, cqe_fp_status_flags;
> >> >             u16 len, pad;
> >> >
> >> >             comp_ring_cons = RCQ_BD(sw_comp_cons);
> >> > @@ -1598,6 +1598,7 @@ static int bnx2x_rx_int(struct bnx2x_fastpath *fp,
> >> int
> >> > budget)
> >> >
> >> >             cqe = &fp->rx_comp_ring[comp_ring_cons];
> >> >             cqe_fp_flags = cqe->fast_path_cqe.type_error_flags;
> >> > +           cqe_fp_status_flags = cqe->fast_path_cqe.status_flags;
> >> >
> >> >             DP(NETIF_MSG_RX_STATUS, "CQE type %x  err %x  status %x"
> >> >                "  queue %x  vlan %x  len %u\n", CQE_TYPE(cqe_fp_flags),
> >> > @@ -1727,6 +1728,12 @@ reuse_rx:
> >> >
> >> >                     skb->protocol = eth_type_trans(skb, bp->dev);
> >> >
> >> > +                   if ((bp->dev->features & ETH_FLAG_RXHASH) &&
> >> > +                       (cqe_fp_status_flags &
> >> > +                        ETH_FAST_PATH_RX_CQE_RSS_HASH_FLG))
> >> > +                           skb->rxhash = le32_to_cpu(
> >> > +                               cqe->fast_path_cqe.rss_hash_result);
> >> > +
> >> >                     skb->ip_summed = CHECKSUM_NONE;
> >> >                     if (bp->rx_csum) {
> >> >                             if (likely(BNX2X_RX_CSUM_OK(cqe)))
> >> > @@ -5750,10 +5757,10 @@ static void bnx2x_init_internal_func(struct
> bnx2x
> >> > *bp)
> >> >     u32 offset;
> >> >     u16 max_agg_size;
> >> >
> >> > -   if (is_multi(bp)) {
> >> > -           tstorm_config.config_flags = MULTI_FLAGS(bp);
> >> > +   tstorm_config.config_flags = RSS_FLAGS(bp);
> >> > +
> >> > +   if (is_multi(bp))
> >> >             tstorm_config.rss_result_mask = MULTI_MASK;
> >> > -   }
> >> >
> >> >     /* Enable TPA if needed */
> >> >     if (bp->flags & TPA_ENABLE_FLAG)
> >> > @@ -6629,10 +6636,8 @@ static int bnx2x_init_common(struct bnx2x *bp)
> >> >     bnx2x_init_block(bp, PBF_BLOCK, COMMON_STAGE);
> >> >
> >> >     REG_WR(bp, SRC_REG_SOFT_RST, 1);
> >> > -   for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4) {
> >> > -           REG_WR(bp, i, 0xc0cac01a);
> >> > -           /* TODO: replace with something meaningful */
> >> > -   }
> >> > +   for (i = SRC_REG_KEYRSS0_0; i <= SRC_REG_KEYRSS1_9; i += 4)
> >> > +           REG_WR(bp, i, random32());
> >> >     bnx2x_init_block(bp, SRCH_BLOCK, COMMON_STAGE);
> >> >  #ifdef BCM_CNIC
> >> >     REG_WR(bp, SRC_REG_KEYSEARCH_0, 0x63285672);
> >> > @@ -11001,6 +11006,11 @@ static int bnx2x_set_flags(struct net_device
> *dev,
> >> > u32 data)
> >> >             changed = 1;
> >> >     }
> >> >
> >> > +   if (data & ETH_FLAG_RXHASH)
> >> > +           dev->features |= NETIF_F_RXHASH;
> >> > +   else
> >> > +           dev->features &= ~NETIF_F_RXHASH;
> >> > +
> >> >     if (changed && netif_running(dev)) {
> >> >             bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> >> >             rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Wed, 7 Jul 2010 12:17:31 -0700

> It is to enable the device to provide the RSS hash in RX descriptor.
> The hash severs two purposes now, it's used internally in the device
> to perform RSS table lookup and also value in RX descriptor.  The
> latter does not require multi-queue.  Strictly speaking, on a single
> processor system without multqueue, it would be true that enabling the
> RX hash on bnx2x is currently superfluous (notwithstanding some other
> use of the hash might be implemented).

We intend to use the card provided RSS hash to optimize GSO
flow comparisons at some point.

There are other possible uses as well.

Therefore even in a single RX queue configuration, the driver
should provide the hash if it can.

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

[Vladislav Zolotarov]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Sunday, July 11, 2010 5:13 AM
> To: therbert@google.com
> Cc: Vladislav Zolotarov; netdev@vger.kernel.org
> Subject: Re: [PATCH] bnx2x: add support for receive hashing
> 
> From: Tom Herbert <therbert@google.com>
> Date: Wed, 7 Jul 2010 12:17:31 -0700
> 
> > It is to enable the device to provide the RSS hash in RX descriptor.
> > The hash severs two purposes now, it's used internally in the device
> > to perform RSS table lookup and also value in RX descriptor.  The
> > latter does not require multi-queue.  Strictly speaking, on a single
> > processor system without multqueue, it would be true that enabling the
> > RX hash on bnx2x is currently superfluous (notwithstanding some other
> > use of the hash might be implemented).
> 
> We intend to use the card provided RSS hash to optimize GSO
> flow comparisons at some point.

Could u explain what did u mean here, pls.? GSO is a Tx flow while RSS hash is generated for the ingress traffic.

> 
> There are other possible uses as well.

Could u elaborate, pls?

> 
> Therefore even in a single RX queue configuration, the driver
> should provide the hash if it can.

Providing an RSS hash adds an additional work to our FW and HW. We would like to understand why do we need to pay this penalty. And surely we don't want to pay it for a possible use. We'd prefer to pay it when it's really needed.

Thanks,
vlad

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

[Vladislav Zolotarov]

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

Thanks,
vlad

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Vladislav Zolotarov
> Sent: Sunday, July 11, 2010 1:02 PM
> To: David Miller; therbert@google.com
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] bnx2x: add support for receive hashing
> 
> 
> 
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Sunday, July 11, 2010 5:13 AM
> > To: therbert@google.com
> > Cc: Vladislav Zolotarov; netdev@vger.kernel.org
> > Subject: Re: [PATCH] bnx2x: add support for receive hashing
> >
> > From: Tom Herbert <therbert@google.com>
> > Date: Wed, 7 Jul 2010 12:17:31 -0700
> >
> > > It is to enable the device to provide the RSS hash in RX descriptor.
> > > The hash severs two purposes now, it's used internally in the device
> > > to perform RSS table lookup and also value in RX descriptor.  The
> > > latter does not require multi-queue.  Strictly speaking, on a single
> > > processor system without multqueue, it would be true that enabling the
> > > RX hash on bnx2x is currently superfluous (notwithstanding some other
> > > use of the hash might be implemented).
> >
> > We intend to use the card provided RSS hash to optimize GSO
> > flow comparisons at some point.
> 
> Could u explain what did u mean here, pls.? GSO is a Tx flow while RSS hash
> is generated for the ingress traffic.
> 
> >
> > There are other possible uses as well.
> 
> Could u elaborate, pls?
> 
> >
> > Therefore even in a single RX queue configuration, the driver
> > should provide the hash if it can.
> 
> Providing an RSS hash adds an additional work to our FW and HW. We would like
> to understand why do we need to pay this penalty. And surely we don't want to
> pay it for a possible use. We'd prefer to pay it when it's really needed.
> 
> Thanks,
> vlad
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

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.

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.

The conditional in driver rx is cheap, since the condition is the same
for all packets and CPU can predicts the branch.

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

[Vladislav Zolotarov]

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

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

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

> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

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.

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

[Vladislav Zolotarov]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Sunday, July 11, 2010 8:42 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 à 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.

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

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

[David Miller]

From: "Vladislav Zolotarov" <vladz@broadcom.com>
Date: Sun, 11 Jul 2010 03:02:17 -0700

>> We intend to use the card provided RSS hash to optimize GSO
>> flow comparisons at some point.
> 
> Could u explain what did u mean here, pls.? GSO is a Tx flow while
> RSS hash is generated for the ingress traffic.

I of course meant GRO, not GSO.