[PATCH net-next] bnx2x: fix GRO parameters

[PATCH net-next] bnx2x: fix GRO parameters

[Yuval Mintz]

bnx2x does an internal GRO pass but doesn't provide gso_segs, thus
breaking qdisc_pkt_len_init() in case ingress qdisc is used.

We store gso_segs in NAPI_GRO_CB(skb)->count, where tcp_gro_complete()
expects to find the number of aggregated segments.

Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
---
This is Eric Dumazet's patch with slight semantic modifications.
Eric - Do you want to put your ack or signoff on this patch?

Dave - Please apply this to 'net-next' afterwards.

Thanks,
Yuval Mintz
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 47 ++++++++++++-------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 18fc26e..49810a0 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -439,31 +439,34 @@ static void bnx2x_tpa_start(struct bnx2x_fastpath *fp, u16 queue,
  */
 #define TPA_TSTAMP_OPT_LEN	12
 /**
- * bnx2x_set_lro_mss - calculate the approximate value of the MSS
+ * bnx2x_set_gro_params - compute GRO values
  *
- * @bp:			driver handle
+ * @skb:		packet skb
  * @parsing_flags:	parsing flags from the START CQE
  * @len_on_bd:		total length of the first packet for the
  *			aggregation.
+ * @pkt_len:		length of all segments
  *
  * Approximate value of the MSS for this aggregation calculated using
  * the first packet of it.
+ * Compute number of aggregated segments, and gso_type
  */
-static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
-			     u16 len_on_bd)
+static void bnx2x_set_gro_params(struct sk_buff *skb, u16 parsing_flags,
+				 u16 len_on_bd, unsigned int pkt_len)
 {
-	/*
-	 * TPA arrgregation won't have either IP options or TCP options
+	/* TPA aggregation won't have either IP options or TCP options
 	 * other than timestamp or IPv6 extension headers.
 	 */
 	u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);
 
 	if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
-	    PRS_FLAG_OVERETH_IPV6)
+	    PRS_FLAG_OVERETH_IPV6) {
 		hdrs_len += sizeof(struct ipv6hdr);
-	else /* IPv4 */
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	} else {
 		hdrs_len += sizeof(struct iphdr);
-
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	/* Check if there was a TCP timestamp, if there is it's will
 	 * always be 12 bytes length: nop nop kind length echo val.
@@ -473,7 +476,13 @@ static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
 	if (parsing_flags & PARSING_FLAGS_TIME_STAMP_EXIST_FLAG)
 		hdrs_len += TPA_TSTAMP_OPT_LEN;
 
-	return len_on_bd - hdrs_len;
+	skb_shinfo(skb)->gso_size = len_on_bd - hdrs_len;
+
+	/* tcp_gro_complete() will copy NAPI_GRO_CB(skb)->count
+	 * to skb_shinfo(skb)->gso_segs
+	 */
+	NAPI_GRO_CB(skb)->count = DIV_ROUND_UP(pkt_len - hdrs_len,
+					       skb_shinfo(skb)->gso_size);
 }
 
 static int bnx2x_alloc_rx_sge(struct bnx2x *bp,
@@ -527,19 +536,9 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	}
 
 	/* This is needed in order to enable forwarding support */
-	if (frag_size) {
-		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(bp,
-					tpa_info->parsing_flags, len_on_bd);
-
-		/* set for GRO */
-		if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size)
-			skb_shinfo(skb)->gso_type =
-			    (GET_FLAG(tpa_info->parsing_flags,
-				      PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
-						PRS_FLAG_OVERETH_IPV6) ?
-				SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
-	}
-
+	if (frag_size)
+		bnx2x_set_gro_params(skb, tpa_info->parsing_flags, len_on_bd,
+				     le16_to_cpu(cqe->pkt_len));
 
 #ifdef BNX2X_STOP_ON_ERROR
 	if (pages > min_t(u32, 8, MAX_SKB_FRAGS)*SGE_PAGE_SIZE*PAGES_PER_SGE) {
@@ -651,7 +650,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 			       struct sk_buff *skb)
 {
 #ifdef CONFIG_INET
-	if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
+	if (skb_shinfo(skb)->gso_size) {
 		skb_set_network_header(skb, 0);
 		switch (be16_to_cpu(skb->protocol)) {
 		case ETH_P_IP:
-- 
1.8.1.227.g44fe835

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Eric Dumazet]

On Thu, 2013-01-17 at 15:26 +0200, Yuval Mintz wrote:
> bnx2x does an internal GRO pass but doesn't provide gso_segs, thus
> breaking qdisc_pkt_len_init() in case ingress qdisc is used.
> 
> We store gso_segs in NAPI_GRO_CB(skb)->count, where tcp_gro_complete()
> expects to find the number of aggregated segments.
> 
> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
> ---
> This is Eric Dumazet's patch with slight semantic modifications.
> Eric - Do you want to put your ack or signoff on this patch?
> 

Sure, thanks !

Signed-off-by: Eric Dumazet <edumazet@google.com>

> Dave - Please apply this to 'net-next' afterwards.
> 
> Thanks,
> Yuval Mintz

Re: [PATCH net-next] bnx2x: fix GRO parameters

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 17 Jan 2013 06:33:01 -0800

> On Thu, 2013-01-17 at 15:26 +0200, Yuval Mintz wrote:
>> bnx2x does an internal GRO pass but doesn't provide gso_segs, thus
>> breaking qdisc_pkt_len_init() in case ingress qdisc is used.
>> 
>> We store gso_segs in NAPI_GRO_CB(skb)->count, where tcp_gro_complete()
>> expects to find the number of aggregated segments.
>> 
>> Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
>> ---
>> This is Eric Dumazet's patch with slight semantic modifications.
>> Eric - Do you want to put your ack or signoff on this patch?
>> 
> 
> Sure, thanks !
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
>> Dave - Please apply this to 'net-next' afterwards.

Applied, thanks.

Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[Yuval Mintz]

On 01/14/2013 08:44 PM, Eric Dumazet wrote:
> On Mon, 2013-01-14 at 09:22 -0800, Eric Dumazet wrote:
> 
> What is the value of gso_segs ?
> 
> The reason I am pointing this out is the recent change in commit
> 1def9238d4aa2146924994aa4b7dc861f03b9362
> (net_sched: more precise pkt_len computation)
> 
> bnx2x not setting gso_segs means that qdisc accounting on ingress is
> completely wrong.

Hi Eric,

First I just want to state that you're totally correct about the gso_segs -
bnx2x is not setting it correctly (it's currently totally omitted), and so
it would incorrectly affect the accounting.

However, notice this behaviour was not introduced in this patch -
Since 621b4d6 bnx2x is using FW GRO, overriding the kernel's GRO implementation:
As the bnx2x driver is supplied with the aggregated packet from its FW,
the bnx2x passes a value in the `gso_size' field of its skb, causing
`skb_is_gso' to return `true'.
This will cause the aggregated skb to override the GRO machinations
(in `dev_gro_receive'), overriding all calls to `gro_receive' and thus also
the call to `skb_gro_receive' which whould have incremented `gso_segs'.

This patch actually tries to correct said behaviour, obviously failing
with the gso_segs but still improving the current state of bnx2x GRO
in bridging scenarios.

>> This looks weird to me. This should be called by GRO stack only.

I think this is the main question - we could try and implement this
inside the network-core itself, but as said behaviour is unique to the
bnx2x driver (correct me if I'm wrong, but I'm unaware of any other
driver which does GRO without the kernel GRO implementation), the
solution is specially tailored for the bnx2x driver.

We could either:
  1. Continue with this patch, later sending a patch correcting gso_segs,
     as this is not a new issue.
  2. Send a V2 patch-series which will also set gso_segs correctly.
  3. Send a V2 patch-series which omits this patch, and later send an RFC
     for some kernel implementation which fixes the issue.

Your thoughts on this matter will be greatly appreciated.

Thanks,
Yuval

Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[Eric Dumazet]

On Tue, 2013-01-15 at 09:28 +0200, Yuval Mintz wrote:
> On 01/14/2013 08:44 PM, Eric Dumazet wrote:
> > On Mon, 2013-01-14 at 09:22 -0800, Eric Dumazet wrote:
> > 
> > What is the value of gso_segs ?
> > 
> > The reason I am pointing this out is the recent change in commit
> > 1def9238d4aa2146924994aa4b7dc861f03b9362
> > (net_sched: more precise pkt_len computation)
> > 
> > bnx2x not setting gso_segs means that qdisc accounting on ingress is
> > completely wrong.
> 
> Hi Eric,
> 
> First I just want to state that you're totally correct about the gso_segs -
> bnx2x is not setting it correctly (it's currently totally omitted), and so
> it would incorrectly affect the accounting.
> 
> However, notice this behaviour was not introduced in this patch -
> Since 621b4d6 bnx2x is using FW GRO, overriding the kernel's GRO implementation:
> As the bnx2x driver is supplied with the aggregated packet from its FW,
> the bnx2x passes a value in the `gso_size' field of its skb, causing
> `skb_is_gso' to return `true'.
> This will cause the aggregated skb to override the GRO machinations
> (in `dev_gro_receive'), overriding all calls to `gro_receive' and thus also
> the call to `skb_gro_receive' which whould have incremented `gso_segs'.
> 
> This patch actually tries to correct said behaviour, obviously failing
> with the gso_segs but still improving the current state of bnx2x GRO
> in bridging scenarios.
> 
> >> This looks weird to me. This should be called by GRO stack only.
> 
> I think this is the main question - we could try and implement this
> inside the network-core itself, but as said behaviour is unique to the
> bnx2x driver (correct me if I'm wrong, but I'm unaware of any other
> driver which does GRO without the kernel GRO implementation), the
> solution is specially tailored for the bnx2x driver.
> 
> We could either:
>   1. Continue with this patch, later sending a patch correcting gso_segs,
>      as this is not a new issue.
>   2. Send a V2 patch-series which will also set gso_segs correctly.
>   3. Send a V2 patch-series which omits this patch, and later send an RFC
>      for some kernel implementation which fixes the issue.
> 
> Your thoughts on this matter will be greatly appreciated.

I am fine with any solution, as long as we fix the problem.

If GRO is done by the FW/driver instead of core network stack, we should
make sure :

- transport_header is set correctly (your patch seems to do that)
- gso_segs is computed (this could be done in core network, but this
adds yet another conditional test in th fast path, and it seems only
bnx2x would need this)

Thanks

Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[Yuval Mintz]

>>> bnx2x not setting gso_segs means that qdisc accounting on ingress is
>>> completely wrong.
>>
>> Notice this behaviour was not introduced in this patch -
>>
>>	...
>>
>> We could either:
>>   1. Continue with this patch, later sending a patch correcting gso_segs,
>>      as this is not a new issue.
>>   2. Send a V2 patch-series which will also set gso_segs correctly.
> 
> I am fine with any solution, as long as we fix the problem.

Eric - Thanks.
Just to be certain - gso_segs should hold the number of non-aggregated packets
contained in the skb's frags, right?


Dave -

Considering Eric's response, following with option (1) or (2) seems like the
right way to go.

Do you want a new patch series which will include a fix for this,
or will you accept a later fix that sets the gso_segs correctly?

(considering this issue is not introduced in this patch,
merely isn't being solved by it)

Thanks,
Yuval Mintz

Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[David Miller]

From: "Yuval Mintz" <yuvalmin@broadcom.com>
Date: Tue, 15 Jan 2013 16:02:32 +0200

> Considering Eric's response, following with option (1) or (2) seems like the
> right way to go.
> 
> Do you want a new patch series which will include a fix for this,
> or will you accept a later fix that sets the gso_segs correctly?
> 
> (considering this issue is not introduced in this patch,
> merely isn't being solved by it)

I've merged your patch series as-is to net-next, please work on fixing
the problems Eric has pointed out.

Thanks.

Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[Eric Dumazet]

On Tue, 2013-01-15 at 15:09 -0500, David Miller wrote:

> 
> I've merged your patch series as-is to net-next, please work on fixing
> the problems Eric has pointed out.

I have two fixes :

One in net/core/dev.c you probably can apply after usual review

One for bnx2x, I guess Broadcom guys need to check it.

[PATCH net-next] bnx2x: fix GRO parameters

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

bnx2x does an internal GRO pass but doesn't provide gso_segs, thus
breaking qdisc_pkt_len_init() in case ingress qdisc is used.

We store gso_segs in NAPI_GRO_CB(skb)->count, where tcp_gro_complete()
expects to find the number of aggregated segments.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yuval Mintz <yuvalmin@broadcom.com>
Cc: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |   44 +++++++-------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 18fc26e..e9bea2e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -439,31 +439,37 @@ static void bnx2x_tpa_start(struct bnx2x_fastpath *fp, u16 queue,
  */
 #define TPA_TSTAMP_OPT_LEN	12
 /**
- * bnx2x_set_lro_mss - calculate the approximate value of the MSS
+ * bnx2x_set_gro_params - compute GRO values
  *
+ * @skb:		packet skb
  * @bp:			driver handle
  * @parsing_flags:	parsing flags from the START CQE
  * @len_on_bd:		total length of the first packet for the
  *			aggregation.
+ * @pkt_len:		length of all segments
  *
  * Approximate value of the MSS for this aggregation calculated using
  * the first packet of it.
+ * Compute number of aggregated segments, and gso_type
  */
-static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
-			     u16 len_on_bd)
+static void bnx2x_set_gro_params(struct sk_buff *skb, struct bnx2x *bp,
+				 u16 parsing_flags, u16 len_on_bd,
+				 unsigned int pkt_len)
 {
 	/*
-	 * TPA arrgregation won't have either IP options or TCP options
+	 * TPA aggregation won't have either IP options or TCP options
 	 * other than timestamp or IPv6 extension headers.
 	 */
 	u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);
 
 	if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
-	    PRS_FLAG_OVERETH_IPV6)
+	    PRS_FLAG_OVERETH_IPV6) {
 		hdrs_len += sizeof(struct ipv6hdr);
-	else /* IPv4 */
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	} else {
 		hdrs_len += sizeof(struct iphdr);
-
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	/* Check if there was a TCP timestamp, if there is it's will
 	 * always be 12 bytes length: nop nop kind length echo val.
@@ -473,7 +479,13 @@ static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
 	if (parsing_flags & PARSING_FLAGS_TIME_STAMP_EXIST_FLAG)
 		hdrs_len += TPA_TSTAMP_OPT_LEN;
 
-	return len_on_bd - hdrs_len;
+	skb_shinfo(skb)->gso_size = len_on_bd - hdrs_len;
+
+	/* tcp_gro_complete() will copy NAPI_GRO_CB(skb)->count
+	 * to skb_shinfo(skb)->gso_segs
+	 */
+	NAPI_GRO_CB(skb)->count = DIV_ROUND_UP(pkt_len - hdrs_len,
+					       skb_shinfo(skb)->gso_size);
 }
 
 static int bnx2x_alloc_rx_sge(struct bnx2x *bp,
@@ -527,18 +539,10 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	}
 
 	/* This is needed in order to enable forwarding support */
-	if (frag_size) {
-		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(bp,
-					tpa_info->parsing_flags, len_on_bd);
+	if (frag_size)
+		bnx2x_set_gro_params(skb, bp, tpa_info->parsing_flags,
+				     len_on_bd, le16_to_cpu(cqe->pkt_len));
 
-		/* set for GRO */
-		if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size)
-			skb_shinfo(skb)->gso_type =
-			    (GET_FLAG(tpa_info->parsing_flags,
-				      PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
-						PRS_FLAG_OVERETH_IPV6) ?
-				SKB_GSO_TCPV6 : SKB_GSO_TCPV4;
-	}
 
 
 #ifdef BNX2X_STOP_ON_ERROR
@@ -651,7 +655,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 			       struct sk_buff *skb)
 {
 #ifdef CONFIG_INET
-	if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
+	if (skb_shinfo(skb)->gso_size) {
 		skb_set_network_header(skb, 0);
 		switch (be16_to_cpu(skb->protocol)) {
 		case ETH_P_IP:

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Yuval Mintz]

> -static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
> -			     u16 len_on_bd)
> +static void bnx2x_set_gro_params(struct sk_buff *skb, struct bnx2x *bp,
> +				 u16 parsing_flags, u16 len_on_bd,
> +				 unsigned int pkt_len)

This is purely semantic, but our convention is for `struct bnx2x' to be
the first argument in our functions.

>  {
>  	/*
> -	 * TPA arrgregation won't have either IP options or TCP options
> +	 * TPA aggregation won't have either IP options or TCP options
>  	 * other than timestamp or IPv6 extension headers.
>  	 */
>  	u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);

TPA_MODE_LRO indicates that an LRO aggregation was made by our FW. It seems
like your patch  eliminates the difference in configuration between the two
(GRO and LRO)

perhaps instead we should do something like:

+ static void bnx2x_set_lro_params(struct bnx2x *bp, struct sk_buff *skb,
+ 				   u16 parsing_flags, u16 len_on_bd,
+ 				   unsigned int pkt_len,
+				   bnx2x_tpa_mode_t mode)

And arrange its suggested code so that only gso_size will be set for LRO.

>  
>  	if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
> -	    PRS_FLAG_OVERETH_IPV6)
> +	    PRS_FLAG_OVERETH_IPV6) {
>  		hdrs_len += sizeof(struct ipv6hdr);
> -	else /* IPv4 */
> +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> +	} else {
>  		hdrs_len += sizeof(struct iphdr);
> -
> +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> +	}
>  



>  #ifdef BNX2X_STOP_ON_ERROR
> @@ -651,7 +655,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
>  			       struct sk_buff *skb)
>  {
>  #ifdef CONFIG_INET
> -	if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
> +	if (skb_shinfo(skb)->gso_size) {

This also seems like an incorrect removal, as TPA_MODE_LRO is (again)
a feasible option, and we wouldn't want the a `gro_complete' here.

>  		skb_set_network_header(skb, 0);
>  		switch (be16_to_cpu(skb->protocol)) {
>  		case ETH_P_IP:
> 

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Eric Dumazet]

On Wed, 2013-01-16 at 09:01 +0200, Yuval Mintz wrote:
> > -static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
> > -			     u16 len_on_bd)
> > +static void bnx2x_set_gro_params(struct sk_buff *skb, struct bnx2x *bp,
> > +				 u16 parsing_flags, u16 len_on_bd,
> > +				 unsigned int pkt_len)
> 
> This is purely semantic, but our convention is for `struct bnx2x' to be
> the first argument in our functions.
> 
> >  {
> >  	/*
> > -	 * TPA arrgregation won't have either IP options or TCP options
> > +	 * TPA aggregation won't have either IP options or TCP options
> >  	 * other than timestamp or IPv6 extension headers.
> >  	 */
> >  	u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);
> 
> TPA_MODE_LRO indicates that an LRO aggregation was made by our FW. It seems
> like your patch  eliminates the difference in configuration between the two
> (GRO and LRO)
> 

Thats the case, since you call GRO functions even if 'LRO ' is ON

I specifically had to remove the tests you guys do.

> perhaps instead we should do something like:
> 
> + static void bnx2x_set_lro_params(struct bnx2x *bp, struct sk_buff *skb,
> + 				   u16 parsing_flags, u16 len_on_bd,
> + 				   unsigned int pkt_len,
> +				   bnx2x_tpa_mode_t mode)
> 
> And arrange its suggested code so that only gso_size will be set for LRO.
> 

I fail to understand why adding a conditional would change something.

Setting it is needed for GRO as well.

> >  
> >  	if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
> > -	    PRS_FLAG_OVERETH_IPV6)
> > +	    PRS_FLAG_OVERETH_IPV6) {
> >  		hdrs_len += sizeof(struct ipv6hdr);
> > -	else /* IPv4 */
> > +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> > +	} else {
> >  		hdrs_len += sizeof(struct iphdr);
> > -
> > +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> > +	}
> >  
> 
> 
> 
> >  #ifdef BNX2X_STOP_ON_ERROR
> > @@ -651,7 +655,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
> >  			       struct sk_buff *skb)
> >  {
> >  #ifdef CONFIG_INET
> > -	if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
> > +	if (skb_shinfo(skb)->gso_size) {
> 
> This also seems like an incorrect removal, as TPA_MODE_LRO is (again)
> a feasible option, and we wouldn't want the a `gro_complete' here.
> 


Problem is : You call GRO functions, faking a GRO packet.

You must therefore exactly present same attributes in skb than after a
true software GRO step.

I did my tests booting a standard driver, that is with LRO on.

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Yuval Mintz]

>>> -static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
>>> -			     u16 len_on_bd)
>>> +static void bnx2x_set_gro_params(struct sk_buff *skb, struct bnx2x *bp,
>>> +				 u16 parsing_flags, u16 len_on_bd,
>>> +				 unsigned int pkt_len)
>>
>> This is purely semantic, but our convention is for `struct bnx2x' to be
>> the first argument in our functions.
>>
>>>  {
>>>  	/*
>>> -	 * TPA arrgregation won't have either IP options or TCP options
>>> +	 * TPA aggregation won't have either IP options or TCP options
>>>  	 * other than timestamp or IPv6 extension headers.
>>>  	 */
>>>  	u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);
>>
>> TPA_MODE_LRO indicates that an LRO aggregation was made by our FW. It seems
>> like your patch  eliminates the difference in configuration between the two
>> (GRO and LRO)
>>
> 
> Thats the case, since you call GRO functions even if 'LRO ' is ON
> 
> I specifically had to remove the tests you guys do.
> 
>> perhaps instead we should do something like:
>>
>> + static void bnx2x_set_lro_params(struct bnx2x *bp, struct sk_buff *skb,
>> + 				   u16 parsing_flags, u16 len_on_bd,
>> + 				   unsigned int pkt_len,
>> +				   bnx2x_tpa_mode_t mode)
>>
>> And arrange its suggested code so that only gso_size will be set for LRO.
>>
> 
> I fail to understand why adding a conditional would change something.
> 
> Setting it is needed for GRO as well.
> 

I was a bit unclear - I meant the for LRO, only gso_size will be set
(while for GRO it will be set as well as the additional GRO fields)

>>>  
>>>  	if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
>>> -	    PRS_FLAG_OVERETH_IPV6)
>>> +	    PRS_FLAG_OVERETH_IPV6) {
>>>  		hdrs_len += sizeof(struct ipv6hdr);
>>> -	else /* IPv4 */
>>> +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>>> +	} else {
>>>  		hdrs_len += sizeof(struct iphdr);
>>> -
>>> +		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
>>> +	}
>>>  
>>
>>
>>
>>>  #ifdef BNX2X_STOP_ON_ERROR
>>> @@ -651,7 +655,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
>>>  			       struct sk_buff *skb)
>>>  {
>>>  #ifdef CONFIG_INET
>>> -	if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
>>> +	if (skb_shinfo(skb)->gso_size) {
>>
>> This also seems like an incorrect removal, as TPA_MODE_LRO is (again)
>> a feasible option, and we wouldn't want the a `gro_complete' here.
>>
> 
> 
> Problem is : You call GRO functions, faking a GRO packet.
> 
> You must therefore exactly present same attributes in skb than after a
> true software GRO step.
> 
> I did my tests booting a standard driver, that is with LRO on.

I think we have a misunderstanding here - when LRO is on, our FW works in
TPA_MODE_LRO (LRO/GRO FW are mutually exclusive). In that mode, the bnx2x
driver will not try to 'fake' GRO packets in the same manner.

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Eric Dumazet]

On Wed, 2013-01-16 at 16:38 +0200, Yuval Mintz wrote:

> I think we have a misunderstanding here - when LRO is on, our FW works in
> TPA_MODE_LRO (LRO/GRO FW are mutually exclusive). In that mode, the bnx2x
> driver will not try to 'fake' GRO packets in the same manner.

I think you didn't really understand the issue then.

Call the hardware assist Receive Offload "LRO" if you really want, even
if not using net/ipv4/inet_lro.c , but provide :

gso_segs and gso_type as well. (Its useful for various things, check
netif_skb_features() for another example)

Or else we need to parse the headers again to check IPv4/ v6 / TCP / UDP
later.

There is no downside providing gso_segs and gso_type, you have all
needed information as shown in the patch I cooked and tested.

Our goal is to remove CONFIG_INET_LRO, so all multi-segments packets
should have the same set of parameters (gso_size, gso_segs, gso_type)

Re: [PATCH net-next] bnx2x: fix GRO parameters

[Yuval Mintz]

> There is no downside providing gso_segs and gso_type, you have all
> needed information as shown in the patch I cooked and tested.
> 
> Our goal is to remove CONFIG_INET_LRO, so all multi-segments packets
> should have the same set of parameters (gso_size, gso_segs, gso_type)

I concur - I'll make the semantic modification to your patch and re-submit
it later on today.

Thanks,
Yuval