* [PATCH] ucc_geth: Reset BQL queue when stopping device @ 2019-01-28 9:07 Mathias Thore 2019-01-28 9:48 ` Christophe Leroy 2019-01-30 18:37 ` David Miller 0 siblings, 2 replies; 8+ messages in thread From: Mathias Thore @ 2019-01-28 9:07 UTC (permalink / raw) To: leoyang.li, netdev, linuxppc-dev, david.gounaris, joakim.tjernlund Cc: Mathias Thore After a timeout event caused by for example a broadcast storm, when the MAC and PHY are reset, the BQL TX queue needs to be reset as well. Otherwise, the device will exhibit severe performance issues even after the storm has ended. Co-authored-by: David Gounaris <david.gounaris@infinera.com> Signed-off-by: Mathias Thore <mathias.thore@infinera.com> --- drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index c3d539e209ed..eb3e65e8868f 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) u16 i, j; u8 __iomem *bd; + netdev_reset_queue(ugeth->ndev); + ug_info = ugeth->ug_info; uf_info = &ug_info->uf_info; -- 2.19.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-28 9:07 [PATCH] ucc_geth: Reset BQL queue when stopping device Mathias Thore @ 2019-01-28 9:48 ` Christophe Leroy 2019-01-28 14:36 ` Mathias Thore 2019-01-30 18:37 ` David Miller 1 sibling, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2019-01-28 9:48 UTC (permalink / raw) To: Mathias Thore, leoyang.li, netdev, linuxppc-dev, david.gounaris, joakim.tjernlund Hi, Le 28/01/2019 à 10:07, Mathias Thore a écrit : > After a timeout event caused by for example a broadcast storm, when > the MAC and PHY are reset, the BQL TX queue needs to be reset as > well. Otherwise, the device will exhibit severe performance issues > even after the storm has ended. What are the symptomns ? Is this reset needed on any network driver in that case, or is it something particular for the ucc_geth ? For instance, the freescale fs_enet doesn't have that reset. Should it have it too ? Christophe > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> > --- > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c > index c3d539e209ed..eb3e65e8868f 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.c > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) > u16 i, j; > u8 __iomem *bd; > > + netdev_reset_queue(ugeth->ndev); > + > ug_info = ugeth->ug_info; > uf_info = &ug_info->uf_info; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-28 9:48 ` Christophe Leroy @ 2019-01-28 14:36 ` Mathias Thore 2019-01-28 21:36 ` Li Yang 0 siblings, 1 reply; 8+ messages in thread From: Mathias Thore @ 2019-01-28 14:36 UTC (permalink / raw) To: Christophe Leroy, leoyang.li, netdev, linuxppc-dev, David Gounaris, Joakim Tjernlund Hi, This is what we observed: there was a storm on the medium so that our controller could not do its TX, resulting in timeout. When timeout occurs, the driver clears all descriptors from the TX queue. The function called in this patch is used to reflect this clearing also in the BQL layer. Without it, the controller would get stuck, unable to perform TX, even several minutes after the storm had ended. Bringing the device down and then up again would solve the problem, but this patch also solves it automatically. Some other drivers do the same, for example e1000e driver calls netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that other drivers should do the same; I have no way of verifying this. Regards, Mathias -- From: Christophe Leroy <christophe.leroy@c-s.fr> Sent: Monday, January 28, 2019 10:48 AM To: Mathias Thore; leoyang.li@nxp.com; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; David Gounaris; Joakim Tjernlund Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. Hi, Le 28/01/2019 à 10:07, Mathias Thore a écrit : > After a timeout event caused by for example a broadcast storm, when > the MAC and PHY are reset, the BQL TX queue needs to be reset as > well. Otherwise, the device will exhibit severe performance issues > even after the storm has ended. What are the symptomns ? Is this reset needed on any network driver in that case, or is it something particular for the ucc_geth ? For instance, the freescale fs_enet doesn't have that reset. Should it have it too ? Christophe > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> > --- > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c > index c3d539e209ed..eb3e65e8868f 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.c > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) > u16 i, j; > u8 __iomem *bd; > > + netdev_reset_queue(ugeth->ndev); > + > ug_info = ugeth->ug_info; > uf_info = &ug_info->uf_info; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-28 14:36 ` Mathias Thore @ 2019-01-28 21:36 ` Li Yang 2019-01-29 8:07 ` Mathias Thore 0 siblings, 1 reply; 8+ messages in thread From: Li Yang @ 2019-01-28 21:36 UTC (permalink / raw) To: Mathias Thore; +Cc: netdev, linuxppc-dev, David Gounaris On Mon, Jan 28, 2019 at 8:37 AM Mathias Thore <Mathias.Thore@infinera.com> wrote: > > Hi, > > > This is what we observed: there was a storm on the medium so that our controller could not do its TX, resulting in timeout. When timeout occurs, the driver clears all descriptors from the TX queue. The function called in this patch is used to reflect this clearing also in the BQL layer. Without it, the controller would get stuck, unable to perform TX, even several minutes after the storm had ended. Bringing the device down and then up again would solve the problem, but this patch also solves it automatically. The explanation makes sense. So this should only be required in the timeout scenario instead of other clean up scenarios like device shutdown? If so, it probably it will be better to be done in ucc_geth_timeout_work()? > > > Some other drivers do the same, for example e1000e driver calls netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that other drivers should do the same; I have no way of verifying this. > > > Regards, > > Mathias > > -- > > > From: Christophe Leroy <christophe.leroy@c-s.fr> > Sent: Monday, January 28, 2019 10:48 AM > To: Mathias Thore; leoyang.li@nxp.com; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; David Gounaris; Joakim Tjernlund > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Hi, > > Le 28/01/2019 à 10:07, Mathias Thore a écrit : > > After a timeout event caused by for example a broadcast storm, when > > the MAC and PHY are reset, the BQL TX queue needs to be reset as > > well. Otherwise, the device will exhibit severe performance issues > > even after the storm has ended. > > What are the symptomns ? > > Is this reset needed on any network driver in that case, or is it > something particular for the ucc_geth ? > For instance, the freescale fs_enet doesn't have that reset. Should it > have it too ? > > Christophe > > > > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> > > --- > > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c > > index c3d539e209ed..eb3e65e8868f 100644 > > --- a/drivers/net/ethernet/freescale/ucc_geth.c > > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) > > u16 i, j; > > u8 __iomem *bd; > > > > + netdev_reset_queue(ugeth->ndev); > > + > > ug_info = ugeth->ug_info; > > uf_info = &ug_info->uf_info; > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-28 21:36 ` Li Yang @ 2019-01-29 8:07 ` Mathias Thore 2019-01-29 19:50 ` Li Yang 2019-01-29 23:33 ` David Miller 0 siblings, 2 replies; 8+ messages in thread From: Mathias Thore @ 2019-01-29 8:07 UTC (permalink / raw) To: Li Yang; +Cc: netdev, linuxppc-dev, David Gounaris Is there a scenario where we are clearing the TX ring but don't want to reset the BQL TX queue? I think it makes sense to keep it in ucc_geth_free_tx since the reason it is needed isn't the timeout per se, but rather the clearing of the TX ring. This way, it will be performed no matter why the driver ends up calling this function. -----Original Message----- From: Li Yang [mailto:leoyang.li@nxp.com] Sent: Monday, 28 January 2019 22:37 To: Mathias Thore <Mathias.Thore@infinera.com> Cc: Christophe Leroy <christophe.leroy@c-s.fr>; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; David Gounaris <David.Gounaris@infinera.com>; Joakim Tjernlund <Joakim.Tjernlund@infinera.com> Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device On Mon, Jan 28, 2019 at 8:37 AM Mathias Thore <Mathias.Thore@infinera.com> wrote: > > Hi, > > > This is what we observed: there was a storm on the medium so that our controller could not do its TX, resulting in timeout. When timeout occurs, the driver clears all descriptors from the TX queue. The function called in this patch is used to reflect this clearing also in the BQL layer. Without it, the controller would get stuck, unable to perform TX, even several minutes after the storm had ended. Bringing the device down and then up again would solve the problem, but this patch also solves it automatically. The explanation makes sense. So this should only be required in the timeout scenario instead of other clean up scenarios like device shutdown? If so, it probably it will be better to be done in ucc_geth_timeout_work()? > > > Some other drivers do the same, for example e1000e driver calls netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that other drivers should do the same; I have no way of verifying this. > > > Regards, > > Mathias > > -- > > > From: Christophe Leroy <christophe.leroy@c-s.fr> > Sent: Monday, January 28, 2019 10:48 AM > To: Mathias Thore; leoyang.li@nxp.com; netdev@vger.kernel.org; > linuxppc-dev@lists.ozlabs.org; David Gounaris; Joakim Tjernlund > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Hi, > > Le 28/01/2019 à 10:07, Mathias Thore a écrit : > > After a timeout event caused by for example a broadcast storm, when > > the MAC and PHY are reset, the BQL TX queue needs to be reset as > > well. Otherwise, the device will exhibit severe performance issues > > even after the storm has ended. > > What are the symptomns ? > > Is this reset needed on any network driver in that case, or is it > something particular for the ucc_geth ? > For instance, the freescale fs_enet doesn't have that reset. Should it > have it too ? > > Christophe > > > > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> > > --- > > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c > > b/drivers/net/ethernet/freescale/ucc_geth.c > > index c3d539e209ed..eb3e65e8868f 100644 > > --- a/drivers/net/ethernet/freescale/ucc_geth.c > > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) > > u16 i, j; > > u8 __iomem *bd; > > > > + netdev_reset_queue(ugeth->ndev); > > + > > ug_info = ugeth->ug_info; > > uf_info = &ug_info->uf_info; > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-29 8:07 ` Mathias Thore @ 2019-01-29 19:50 ` Li Yang 2019-01-29 23:33 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: Li Yang @ 2019-01-29 19:50 UTC (permalink / raw) To: Mathias Thore; +Cc: netdev, linuxppc-dev, David Gounaris On Tue, Jan 29, 2019 at 2:09 AM Mathias Thore <Mathias.Thore@infinera.com> wrote: > > Is there a scenario where we are clearing the TX ring but don't want to reset the BQL TX queue? Right now the function is also used on interface open/close, driver removal and driver resumption besides the timeout situation. I think the reseting BQL queue is either not neccessary or already called explicitly for the other scenarios. > > I think it makes sense to keep it in ucc_geth_free_tx since the reason it is needed isn't the timeout per se, but rather the clearing of the TX ring. This way, it will be performed no matter why the driver ends up calling this function. I don't see a consensus on when netdev_reset_queue() should be called among existing drivers. Doing it on buffer ring cleanup probably can be future-proofing. But if we want to use this approach, we can remove the redundent netdev_reset_queue() calls in open/close functions. Regards, Leo > > -----Original Message----- > From: Li Yang [mailto:leoyang.li@nxp.com] > Sent: Monday, 28 January 2019 22:37 > To: Mathias Thore <Mathias.Thore@infinera.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr>; netdev@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; David Gounaris <David.Gounaris@infinera.com>; Joakim Tjernlund <Joakim.Tjernlund@infinera.com> > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > On Mon, Jan 28, 2019 at 8:37 AM Mathias Thore <Mathias.Thore@infinera.com> wrote: > > > > Hi, > > > > > > This is what we observed: there was a storm on the medium so that our controller could not do its TX, resulting in timeout. When timeout occurs, the driver clears all descriptors from the TX queue. The function called in this patch is used to reflect this clearing also in the BQL layer. Without it, the controller would get stuck, unable to perform TX, even several minutes after the storm had ended. Bringing the device down and then up again would solve the problem, but this patch also solves it automatically. > > The explanation makes sense. So this should only be required in the timeout scenario instead of other clean up scenarios like device shutdown? If so, it probably it will be better to be done in ucc_geth_timeout_work()? > > > > > > > Some other drivers do the same, for example e1000e driver calls netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that other drivers should do the same; I have no way of verifying this. > > > > > > Regards, > > > > Mathias > > > > -- > > > > > > From: Christophe Leroy <christophe.leroy@c-s.fr> > > Sent: Monday, January 28, 2019 10:48 AM > > To: Mathias Thore; leoyang.li@nxp.com; netdev@vger.kernel.org; > > linuxppc-dev@lists.ozlabs.org; David Gounaris; Joakim Tjernlund > > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > Hi, > > > > Le 28/01/2019 à 10:07, Mathias Thore a écrit : > > > After a timeout event caused by for example a broadcast storm, when > > > the MAC and PHY are reset, the BQL TX queue needs to be reset as > > > well. Otherwise, the device will exhibit severe performance issues > > > even after the storm has ended. > > > > What are the symptomns ? > > > > Is this reset needed on any network driver in that case, or is it > > something particular for the ucc_geth ? > > For instance, the freescale fs_enet doesn't have that reset. Should it > > have it too ? > > > > Christophe > > > > > > > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > > > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> > > > --- > > > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c > > > b/drivers/net/ethernet/freescale/ucc_geth.c > > > index c3d539e209ed..eb3e65e8868f 100644 > > > --- a/drivers/net/ethernet/freescale/ucc_geth.c > > > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > > > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth) > > > u16 i, j; > > > u8 __iomem *bd; > > > > > > + netdev_reset_queue(ugeth->ndev); > > > + > > > ug_info = ugeth->ug_info; > > > uf_info = &ug_info->uf_info; > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-29 8:07 ` Mathias Thore 2019-01-29 19:50 ` Li Yang @ 2019-01-29 23:33 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2019-01-29 23:33 UTC (permalink / raw) To: Mathias.Thore; +Cc: David.Gounaris, netdev, leoyang.li, linuxppc-dev From: Mathias Thore <Mathias.Thore@infinera.com> Date: Tue, 29 Jan 2019 08:07:54 +0000 > Is there a scenario where we are clearing the TX ring but don't want to reset the BQL TX queue? > > I think it makes sense to keep it in ucc_geth_free_tx since the > reason it is needed isn't the timeout per se, but rather the > clearing of the TX ring. This way, it will be performed no matter > why the driver ends up calling this function. I absolutely think the BQL reset should remain in ucc_geth_free_tx(). That way if another callsite for ucc_geth_free_tx() emerges that does need the BQL queue reset, it won't be "forgotten". ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ucc_geth: Reset BQL queue when stopping device 2019-01-28 9:07 [PATCH] ucc_geth: Reset BQL queue when stopping device Mathias Thore 2019-01-28 9:48 ` Christophe Leroy @ 2019-01-30 18:37 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2019-01-30 18:37 UTC (permalink / raw) To: mathias.thore; +Cc: netdev, linuxppc-dev, david.gounaris, leoyang.li From: Mathias Thore <mathias.thore@infinera.com> Date: Mon, 28 Jan 2019 10:07:47 +0100 > After a timeout event caused by for example a broadcast storm, when > the MAC and PHY are reset, the BQL TX queue needs to be reset as > well. Otherwise, the device will exhibit severe performance issues > even after the storm has ended. > > Co-authored-by: David Gounaris <david.gounaris@infinera.com> > Signed-off-by: Mathias Thore <mathias.thore@infinera.com> Applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-30 18:39 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-28 9:07 [PATCH] ucc_geth: Reset BQL queue when stopping device Mathias Thore 2019-01-28 9:48 ` Christophe Leroy 2019-01-28 14:36 ` Mathias Thore 2019-01-28 21:36 ` Li Yang 2019-01-29 8:07 ` Mathias Thore 2019-01-29 19:50 ` Li Yang 2019-01-29 23:33 ` David Miller 2019-01-30 18:37 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).