| Message ID | 1364967689-11155-1-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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 */
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
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
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 */
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(-)