| Message ID | 1381937052-8999-4-git-send-email-jimmy.perchet@parrot.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 10/16/2013 07:24 PM, Jimmy Perchet wrote: > On low speed link (10MBit/s), some TX descriptors can remain dirty > if the tx coalescence timer expires before they were treated. Re-arm timer > in this case. > Signed-off-by: Jimmy Perchet <jimmy.perchet@parrot.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 0015175..af04b5d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1284,8 +1284,12 @@ static void stmmac_tx_clean(struct stmmac_priv *priv) > p = priv->dma_tx + entry; > > /* Check if the descriptor is owned by the DMA. */ > - if (priv->hw->desc->get_tx_owner(p)) > + if (priv->hw->desc->get_tx_owner(p)) { > + /* Be sure to harvest remaining descriptor. */ > + mod_timer(&priv->txtimer, > + STMMAC_COAL_TIMER(priv->tx_coal_timer)); The continuation line should stat right under &, according to thenetworking coding style. WBR, Sergei -- 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
Hello, > > The continuation line should stat right under &, according to thenetworking coding style. Thanks for your comment, I realized that there is several other issues regarding continuation line in my patch series. I'll fix them in v2. Best Regards, Jimmy -- 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
Hello Jimmy On 10/16/2013 5:24 PM, Jimmy Perchet wrote: > On low speed link (10MBit/s), some TX descriptors can remain dirty > if the tx coalescence timer expires before they were treated. Re-arm timer > in this case. > > Signed-off-by: Jimmy Perchet <jimmy.perchet@parrot.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 0015175..af04b5d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1284,8 +1284,12 @@ static void stmmac_tx_clean(struct stmmac_priv *priv) > p = priv->dma_tx + entry; > > /* Check if the descriptor is owned by the DMA. */ > - if (priv->hw->desc->get_tx_owner(p)) > + if (priv->hw->desc->get_tx_owner(p)) { > + /* Be sure to harvest remaining descriptor. */ > + mod_timer(&priv->txtimer, > + STMMAC_COAL_TIMER(priv->tx_coal_timer)); > break; > + } why should we reload the timer when clean the tx resource? This is done in the xmit where as soon as a frame has to be transmitted it makes sense to reload the timer. Also I have not clear why the problem happens on 10MBit/s speed Maybe there is an hidden problem (lock protection) that should be fixed. How did you get this problem? Just on low speed and stress the net? I have never seen it. peppe > > /* Verify tx error by looking at the last segment. */ > last = priv->hw->desc->get_tx_ls(p); > -- 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
Hello Peppe, On 21/10/2013 11:07, Giuseppe CAVALLARO wrote: > Hello Jimmy > > On 10/16/2013 5:24 PM, Jimmy Perchet wrote: >> On low speed link (10MBit/s), some TX descriptors can remain dirty >> if the tx coalescence timer expires before they were treated. Re-arm timer >> in this case. >> >> Signed-off-by: Jimmy Perchet <jimmy.perchet@parrot.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index 0015175..af04b5d 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -1284,8 +1284,12 @@ static void stmmac_tx_clean(struct stmmac_priv *priv) >> p = priv->dma_tx + entry; >> >> /* Check if the descriptor is owned by the DMA. */ >> - if (priv->hw->desc->get_tx_owner(p)) >> + if (priv->hw->desc->get_tx_owner(p)) { >> + /* Be sure to harvest remaining descriptor. */ >> + mod_timer(&priv->txtimer, >> + STMMAC_COAL_TIMER(priv->tx_coal_timer)); >> break; >> + } > > > why should we reload the timer when clean the tx resource? > This is done in the xmit where as soon as a frame has to be > transmitted it makes sense to reload the timer. > > Also I have not clear why the problem happens on 10MBit/s speed > Maybe there is an hidden problem (lock protection) > that should be fixed. > > How did you get this problem? Just on low speed and stress the net? > I have never seen it. I can reproduce this problem by issuing 9KiB jumbo frames on 10MBit/s link. If socket's wmemory size is about 500kiB (or less), the transfer stall. (I guess it is reproducible with 1500o frames by decreasing socket's wmemory to 90KB) Re-arming the timer fix this behaviour. Here my understanding of this issue : With 9KiB frames and 500kiB of wmemory, only 60 frames can be prepared in a row. It is below the tx coalescence threshold, so there will be no interrupt. When the tx coalescence timer expires (40ms after), only five descriptors have to be freed (9000*5 @ 10Mbit/s = 34ms), it is not enough to reach the socket's wake-up threshold. We get into a deadlock : *Socket is waiting for free buffers before performing new transfer. *Driver is waiting for new transfer before performing cleanup. Maybe, it is not a real life use-case, and is not worth a patch. What do you think ? Best Regards, Jimmy > > peppe > >> >> /* Verify tx error by looking at the last segment. */ >> last = priv->hw->desc->get_tx_ls(p); >> > -- 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 Mon, 2013-10-21 at 15:10 +0200, Jimmy PERCHET wrote: > Hello Peppe, > > I can reproduce this problem by issuing 9KiB jumbo frames on 10MBit/s link. > If socket's wmemory size is about 500kiB (or less), the transfer stall. > (I guess it is reproducible with 1500o frames by decreasing > socket's wmemory to 90KB) > Re-arming the timer fix this behaviour. > > Here my understanding of this issue : > With 9KiB frames and 500kiB of wmemory, only 60 frames can be > prepared in a row. It is below the tx coalescence threshold, > so there will be no interrupt. When the tx coalescence timer > expires (40ms after), only five descriptors have to be > freed (9000*5 @ 10Mbit/s = 34ms), it is not enough to reach > the socket's wake-up threshold. We get into a deadlock : > *Socket is waiting for free buffers before performing new transfer. > *Driver is waiting for new transfer before performing cleanup. > > Maybe, it is not a real life use-case, and is not worth > a patch. What do you think ? > I think there is probably a bug in the driver, a race of some sort, and it would be better to find it and fix it ;) -- 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 Mon, 2013-10-21 at 11:32 -0700, Eric Dumazet wrote: > On Mon, 2013-10-21 at 15:10 +0200, Jimmy PERCHET wrote: > > Hello Peppe, > > > > > I can reproduce this problem by issuing 9KiB jumbo frames on 10MBit/s link. > > If socket's wmemory size is about 500kiB (or less), the transfer stall. > > (I guess it is reproducible with 1500o frames by decreasing > > socket's wmemory to 90KB) > > Re-arming the timer fix this behaviour. > > > > Here my understanding of this issue : > > With 9KiB frames and 500kiB of wmemory, only 60 frames can be > > prepared in a row. It is below the tx coalescence threshold, > > so there will be no interrupt. When the tx coalescence timer > > expires (40ms after), only five descriptors have to be > > freed (9000*5 @ 10Mbit/s = 34ms), it is not enough to reach > > the socket's wake-up threshold. We get into a deadlock : > > *Socket is waiting for free buffers before performing new transfer. > > *Driver is waiting for new transfer before performing cleanup. > > > > Maybe, it is not a real life use-case, and is not worth > > a patch. What do you think ? > > > > I think there is probably a bug in the driver, a race of some sort, > and it would be better to find it and fix it ;) > coalesce params should not be hardcoded, but depend on link speed and mtu. On 10Mbits, and MTU=9000 there is really no point using coalescing ! -- 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 10/21/2013 8:49 PM, Eric Dumazet wrote: > On Mon, 2013-10-21 at 11:32 -0700, Eric Dumazet wrote: >> On Mon, 2013-10-21 at 15:10 +0200, Jimmy PERCHET wrote: >>> Hello Peppe, >>> >> >>> I can reproduce this problem by issuing 9KiB jumbo frames on 10MBit/s link. >>> If socket's wmemory size is about 500kiB (or less), the transfer stall. >>> (I guess it is reproducible with 1500o frames by decreasing >>> socket's wmemory to 90KB) >>> Re-arming the timer fix this behaviour. >>> >>> Here my understanding of this issue : >>> With 9KiB frames and 500kiB of wmemory, only 60 frames can be >>> prepared in a row. It is below the tx coalescence threshold, >>> so there will be no interrupt. When the tx coalescence timer >>> expires (40ms after), only five descriptors have to be >>> freed (9000*5 @ 10Mbit/s = 34ms), it is not enough to reach >>> the socket's wake-up threshold. We get into a deadlock : >>> *Socket is waiting for free buffers before performing new transfer. >>> *Driver is waiting for new transfer before performing cleanup. >>> >>> Maybe, it is not a real life use-case, and is not worth >>> a patch. What do you think ? >>> >> >> I think there is probably a bug in the driver, a race of some sort, >> and it would be better to find it and fix it ;) >> > > coalesce params should not be hardcoded, but depend on link speed and > mtu. > > On 10Mbits, and MTU=9000 there is really no point using coalescing ! so the final patch could be to tune/disable the tx coalesce according to speed and mtu. Indeed I had added something that can already help on that. We can tune the tx_coal_frames and decide to set the IC bit (interrupt on completion bit) in the frame to be transmitted. This can be done via ethtool. This should reduce the mitigation so, for sure, you can tune all in case of low speed or jumbo. IIRC, you could decide to disable mitigation at all. To Jimmy, can you try this? In any case, let me know. 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
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 0015175..af04b5d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1284,8 +1284,12 @@ static void stmmac_tx_clean(struct stmmac_priv *priv) p = priv->dma_tx + entry; /* Check if the descriptor is owned by the DMA. */ - if (priv->hw->desc->get_tx_owner(p)) + if (priv->hw->desc->get_tx_owner(p)) { + /* Be sure to harvest remaining descriptor. */ + mod_timer(&priv->txtimer, + STMMAC_COAL_TIMER(priv->tx_coal_timer)); break; + } /* Verify tx error by looking at the last segment. */ last = priv->hw->desc->get_tx_ls(p);
On low speed link (10MBit/s), some TX descriptors can remain dirty if the tx coalescence timer expires before they were treated. Re-arm timer in this case. Signed-off-by: Jimmy Perchet <jimmy.perchet@parrot.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)