| Message ID | CAHrpEqSi+caO5gSo1BZRGqytxhCOs-OuyjiCaG+7fnsnFBj77g@mail.gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Frank Li <lznuaa@gmail.com> : [...] > diff --git a/drivers/net/ethernet/freescale/fec.c > b/drivers/net/ethernet/freescale/fe > index 73195f6..c945bb7 100644 > --- a/drivers/net/ethernet/freescale/fec.c > +++ b/drivers/net/ethernet/freescale/fec.c > @@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex) > const struct platform_device_id *id_entry = > platform_get_device_id(fep->pdev); > int i; > + if (netif_running(ndev)) { > + napi_disable(&fep->napi); napi_disable may sleep. fec_restart can be called with spinlock held in fec_enet_adjust_link
2013/4/27, Francois Romieu <romieu@fr.zoreil.com>: > Frank Li <lznuaa@gmail.com> : > [...] >> diff --git a/drivers/net/ethernet/freescale/fec.c >> b/drivers/net/ethernet/freescale/fe >> index 73195f6..c945bb7 100644 >> --- a/drivers/net/ethernet/freescale/fec.c >> +++ b/drivers/net/ethernet/freescale/fec.c >> @@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex) >> const struct platform_device_id *id_entry = >> platform_get_device_id(fep->pdev); >> int i; >> + if (netif_running(ndev)) { >> + napi_disable(&fep->napi); > > napi_disable may sleep. > > fec_restart can be called with spinlock held in fec_enet_adjust_link You are right. Spin lock in FEC enet adjust link can be removed when remove spinlock in tx and RX. > > -- > Ueimor > -- 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
Frank Li <lznuaa@gmail.com> : > 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>: [...] > > napi_disable may sleep. > > > > fec_restart can be called with spinlock held in fec_enet_adjust_link > > You are right. Spin lock in FEC enet adjust link can be removed when > remove spinlock in tx and RX. fec_restart is also called from the netdev watchdog handler (fec_timeout) and tasklet can't sleep either. You should imho schedule_work from fec_timeout. How is the driver supposed to avoid the napi context fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue race since both can run on different cpu without any read/write ordering enforcement between one thread netif_* and its peer {dirty/cur}_tx ?
2013/4/28 Francois Romieu <romieu@fr.zoreil.com>: > Frank Li <lznuaa@gmail.com> : >> 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>: > [...] >> > napi_disable may sleep. >> > >> > fec_restart can be called with spinlock held in fec_enet_adjust_link >> >> You are right. Spin lock in FEC enet adjust link can be removed when >> remove spinlock in tx and RX. > > fec_restart is also called from the netdev watchdog handler (fec_timeout) > and tasklet can't sleep either. You should imho schedule_work from > fec_timeout. I will add delay_work to fix this issue. > > How is the driver supposed to avoid the napi context > fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue > race since both can run on different cpu without any read/write ordering > enforcement between one thread netif_* and its peer {dirty/cur}_tx ? This is lockless design if only one read and one write. It likes kfifo. Assume CPU1 run xmit. CPU2 run napi fec_enet_tx (1) if (fep->cur_tx == fep->dirty_tx) (2) netif_stop_queue(ndev); if CPU2 update fep->dirty_tx before CPU1 run (1). the condition is false. if CPU2 update fep->dirty_tx after (1) before (2), netif_stop_queue will be called. There are not problem at this time even though queue is not full here. queue is not empty for sure here. fec_enet_tx will be called again when NAPI trigger by one frame finished transfer, fec_enet_tx will wake up send queue. Most worse case is waste one BD script. > > -- > Ueimor -- 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
2013/4/28 Frank Li <lznuaa@gmail.com>: > 2013/4/28 Francois Romieu <romieu@fr.zoreil.com>: >> Frank Li <lznuaa@gmail.com> : >>> 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>: >> [...] >>> > napi_disable may sleep. >>> > >>> > fec_restart can be called with spinlock held in fec_enet_adjust_link >>> >>> You are right. Spin lock in FEC enet adjust link can be removed when >>> remove spinlock in tx and RX. >> >> fec_restart is also called from the netdev watchdog handler (fec_timeout) >> and tasklet can't sleep either. You should imho schedule_work from >> fec_timeout. > > I will add delay_work to fix this issue. > >> >> How is the driver supposed to avoid the napi context >> fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue >> race since both can run on different cpu without any read/write ordering >> enforcement between one thread netif_* and its peer {dirty/cur}_tx ? > > This is lockless design if only one read and one write. It likes kfifo. > > Assume CPU1 run xmit. CPU2 run napi fec_enet_tx > > (1) if (fep->cur_tx == fep->dirty_tx) > (2) netif_stop_queue(ndev); > > if CPU2 update fep->dirty_tx before CPU1 run (1). the condition is false. > if CPU2 update fep->dirty_tx after (1) before (2), netif_stop_queue > will be called. There are not problem at this time even though queue > is not full here. queue is not empty for sure here. fec_enet_tx will > be called again when NAPI trigger by one frame finished transfer, > fec_enet_tx will wake up send queue. > > Most worse case is waste one BD script. I just send out a patch to try to fix your problem. Can you help test it in your sit? > >> >> -- >> Ueimor -- 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
Frank Li <lznuaa@gmail.com> : [...] > This is lockless design if only one read and one write. It likes kfifo. > > Assume CPU1 run xmit. CPU2 run napi fec_enet_tx > > (1) if (fep->cur_tx == fep->dirty_tx) > (2) netif_stop_queue(ndev); > > if CPU2 update fep->dirty_tx before CPU1 run (1). the condition is false. > if CPU2 update fep->dirty_tx after (1) before (2), netif_stop_queue > will be called. There are not problem at this time even though queue > is not full here. queue is not empty for sure here. fec_enet_tx will > be called again when NAPI trigger by one frame finished transfer, > fec_enet_tx will wake up send queue. "before" and "after" may not work as expected between different CPU without explicit synchronization (or barrier). It won't oops but it would be careful to envision something like drivers/net/ethernet/broadcom/tg3.c::tg3_tx.
Frank Li <lznuaa@gmail.com> : [...] > I just send out a patch to try to fix your problem. > Can you help test it in your sit? I don't have the required hardware. But I can find bugs while the napi-is-too-hard-please-do-proper-project-mgmt squad watches elsewhere. :o)
2013/4/28 Francois Romieu <romieu@fr.zoreil.com>: > Frank Li <lznuaa@gmail.com> : > [...] >> I just send out a patch to try to fix your problem. >> Can you help test it in your sit? > > I don't have the required hardware. > > But I can find bugs while the napi-is-too-hard-please-do-proper-project-mgmt > squad watches elsewhere. :o) Sorry, what's your means? > > -- > Ueimor -- 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
2013/4/28 Francois Romieu <romieu@fr.zoreil.com>: > Frank Li <lznuaa@gmail.com> : > [...] >> This is lockless design if only one read and one write. It likes kfifo. >> >> Assume CPU1 run xmit. CPU2 run napi fec_enet_tx >> >> (1) if (fep->cur_tx == fep->dirty_tx) >> (2) netif_stop_queue(ndev); >> >> if CPU2 update fep->dirty_tx before CPU1 run (1). the condition is false. >> if CPU2 update fep->dirty_tx after (1) before (2), netif_stop_queue >> will be called. There are not problem at this time even though queue >> is not full here. queue is not empty for sure here. fec_enet_tx will >> be called again when NAPI trigger by one frame finished transfer, >> fec_enet_tx will wake up send queue. > > "before" and "after" may not work as expected between different CPU without > explicit synchronization (or barrier). It won't oops but it would be careful > to envision something like drivers/net/ethernet/broadcom/tg3.c::tg3_tx. tg3_tx is safe method but need more lock. But I think it is not related with this issue. I can change later. > > -- > Ueimor -- 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
Frank Li <lznuaa@gmail.com> : [...] > tg3_tx is safe method but need more lock. It's the same design. > But I think it is not related with this issue. I can change later. It won't oops, sure. It will simply stop under high load or proper timing.
2013/4/29, Francois Romieu <romieu@fr.zoreil.com>: > Frank Li <lznuaa@gmail.com> : > [...] >> tg3_tx is safe method but need more lock. > > It's the same design. > >> But I think it is not related with this issue. I can change later. > > It won't oops, sure. It will simply stop under high load or proper > timing. It is difference problem. It should be new patch. Do you have test step to reproduce the problem what you said? > > -- > Ueimor > -- 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
Am Samstag, den 27.04.2013, 18:26 +0800 schrieb Frank Li: > > > > - remove locking in a way that memory is freed which is in use > > The purpose of remove lock is that fix dead lock. Could you please elaborate? What is the exact situation which caused a deadlock here? I tried to see what's happening from your changelogs, but I'm not sure I can follow here. Wasn't the problem more of the used spinlocks not being IRQ-save and thus potentially creating a deadlock? Regards, Lucas
2013/4/29 Lucas Stach <l.stach@pengutronix.de>: > What is the exact situation which caused a > deadlock here? I tried to see what's happening from your changelogs, but > I'm not sure I can follow here. Wasn't the problem more of the used > spinlocks not being IRQ-save and thus potentially creating a deadlock? You can view below email thread. http://www.spinics.net/lists/netdev/msg225240.html The main problem network API use spin_lock_bh only now. So it does not prefer use irq to handle skb process. -- 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/freescale/fec.c b/drivers/net/ethernet/freescale/fe index 73195f6..c945bb7 100644 --- a/drivers/net/ethernet/freescale/fec.c +++ b/drivers/net/ethernet/freescale/fec.c @@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex) const struct platform_device_id *id_entry = platform_get_device_id(fep->pdev); int i; + if (netif_running(ndev)) { + napi_disable(&fep->napi); + netif_stop_queue(ndev); + } + u32 temp_mac[2]; u32 rcntl = OPT_FRAME_SIZE | 0x04; u32 ecntl = 0x2; /* ETHEREN */ @@ -559,6 +564,11 @@ fec_restart(struct net_device *ndev, int duplex) /* Enable interrupts we wish to service */ writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); + + if (netif_running(ndev)) { + napi_enable(&fep->napi); + netif_wake_queue(ndev); + } }