Patchwork [net-next.git,1/7] stmmac: review napi gro support

login
register
mail settings
Submitter Giuseppe CAVALLARO
Date April 3, 2013, 5:41 a.m.
Message ID <1364967689-11155-1-git-send-email-peppe.cavallaro@st.com>
Download mbox | patch
Permalink /patch/233254/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Giuseppe CAVALLARO - April 3, 2013, 5:41 a.m.
This patch is to:
o use napi_gro_flush() before calling __napi_complete()
o turn on NETIF_F_GRO by default

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Eric Dumazet - April 3, 2013, 7:05 a.m.
On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
> This patch is to:
> o use napi_gro_flush() before calling __napi_complete()
> o turn on NETIF_F_GRO by default
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6b26d31..8b69e3b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2046,7 +2046,8 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
>  
>  	work_done = stmmac_rx(priv, budget);
>  	if (work_done < budget) {
> -		napi_complete(napi);
> +		napi_gro_flush(napi, false);
> +		__napi_complete(napi);
>  		stmmac_enable_dma_irq(priv);
>  	}

Why are you doing this ?

This adds a (fatal) race.


--
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
Giuseppe CAVALLARO - April 3, 2013, 7:41 a.m.
On 4/3/2013 9:05 AM, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
>> This patch is to:
>> o use napi_gro_flush() before calling __napi_complete()
>> o turn on NETIF_F_GRO by default
>>
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++--
>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 6b26d31..8b69e3b 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2046,7 +2046,8 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
>>
>>   	work_done = stmmac_rx(priv, budget);
>>   	if (work_done < budget) {
>> -		napi_complete(napi);
>> +		napi_gro_flush(napi, false);
>> +		__napi_complete(napi);
>>   		stmmac_enable_dma_irq(priv);
>>   	}
>
> Why are you doing this ?
>
> This adds a (fatal) race.

Hmm, I'm in trouble on this :-). Indeed I can understand the (fatal)
race and why napi_complete should be used. Sorry! So my fault and this 
patch has to be discarded. I don't understand why I have not seen any
problems while running/stressing on SMP system. Have you got any idea?

Thanks Eric for your prompt feedback.
Let me know if you see other problems so I'll try to fix all soon.

peppe
--
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
Eric Dumazet - April 3, 2013, 1:39 p.m.
On Wed, 2013-04-03 at 09:41 +0200, Giuseppe CAVALLARO wrote:

> Hmm, I'm in trouble on this :-). Indeed I can understand the (fatal)
> race and why napi_complete should be used. Sorry! So my fault and this 
> patch has to be discarded. I don't understand why I have not seen any
> problems while running/stressing on SMP system. Have you got any idea?
> 

So because you don't hit the race on your machine and your tests, we can
remove all the needed spinlock() and various hard irq masking, and
introduce all sort of races ?

Try to use a combination of two NICS, and you'll hit the bug even on UP.

There is a single poll_list per cpu, and we insert new elements in this
list under hard irq.

So deleting elements _must_ be done under the protection of hard irq
masking.




--
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
Ben Hutchings - April 3, 2013, 4:01 p.m.
On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
> This patch is to:
> o use napi_gro_flush() before calling __napi_complete()

napi_complete() already does that, and some other important things.  Why
do you think it makes sense to call __napi_complete() directly?

Ben.

> o turn on NETIF_F_GRO by default
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6b26d31..8b69e3b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2046,7 +2046,8 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
>  
>  	work_done = stmmac_rx(priv, budget);
>  	if (work_done < budget) {
> -		napi_complete(napi);
> +		napi_gro_flush(napi, false);
> +		__napi_complete(napi);
>  		stmmac_enable_dma_irq(priv);
>  	}
>  	return work_done;
> @@ -2586,7 +2587,7 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  
>  	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  			    NETIF_F_RXCSUM;
> -	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
> +	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA | NETIF_F_GRO;
>  	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
>  	/* Both mac100 and gmac support receive VLAN tag detection */
Giuseppe CAVALLARO - April 4, 2013, 6:16 a.m.
On 4/3/2013 3:39 PM, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 09:41 +0200, Giuseppe CAVALLARO wrote:
>
>> Hmm, I'm in trouble on this :-). Indeed I can understand the (fatal)
>> race and why napi_complete should be used. Sorry! So my fault and this
>> patch has to be discarded. I don't understand why I have not seen any
>> problems while running/stressing on SMP system. Have you got any idea?
>>
>
> So because you don't hit the race on your machine and your tests, we can
> remove all the needed spinlock() and various hard irq masking, and
> introduce all sort of races ?

No, no this was not my intention. If fact, I asked to discard the patch
;-) ... it is clear to me the race that can happen in that case.

I'll rework the patch to only add the GRO in the feature.

peppe

>
> Try to use a combination of two NICS, and you'll hit the bug even on UP.
>
> There is a single poll_list per cpu, and we insert new elements in this
> list under hard irq.
>
> So deleting elements _must_ be done under the protection of hard irq
> masking.
>
>
>
>
>

--
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
Giuseppe CAVALLARO - April 4, 2013, 6:20 a.m.
On 4/3/2013 6:01 PM, Ben Hutchings wrote:
> On Wed, 2013-04-03 at 07:41 +0200, Giuseppe CAVALLARO wrote:
>> This patch is to:
>> o use napi_gro_flush() before calling __napi_complete()
>
> napi_complete() already does that, and some other important things.  Why
> do you think it makes sense to call __napi_complete() directly?

yes Ben you are right. In fact Eric already alerted me on this.
It is my fault, I'll remove this in my next set.

>
> Ben.
>
>> o turn on NETIF_F_GRO by default
>>
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    5 +++--
>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 6b26d31..8b69e3b 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2046,7 +2046,8 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
>>
>>   	work_done = stmmac_rx(priv, budget);
>>   	if (work_done < budget) {
>> -		napi_complete(napi);
>> +		napi_gro_flush(napi, false);
>> +		__napi_complete(napi);
>>   		stmmac_enable_dma_irq(priv);
>>   	}
>>   	return work_done;
>> @@ -2586,7 +2587,7 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>
>>   	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>   			    NETIF_F_RXCSUM;
>> -	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
>> +	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA | NETIF_F_GRO;
>>   	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>>   #ifdef STMMAC_VLAN_TAG_USED
>>   	/* Both mac100 and gmac support receive VLAN tag detection */
>

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

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6b26d31..8b69e3b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2046,7 +2046,8 @@  static int stmmac_poll(struct napi_struct *napi, int budget)
 
 	work_done = stmmac_rx(priv, budget);
 	if (work_done < budget) {
-		napi_complete(napi);
+		napi_gro_flush(napi, false);
+		__napi_complete(napi);
 		stmmac_enable_dma_irq(priv);
 	}
 	return work_done;
@@ -2586,7 +2587,7 @@  struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 
 	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			    NETIF_F_RXCSUM;
-	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
+	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA | NETIF_F_GRO;
 	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
 #ifdef STMMAC_VLAN_TAG_USED
 	/* Both mac100 and gmac support receive VLAN tag detection */