| Message ID | 1425937937-26955-3-git-send-email-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2015-03-09 at 14:52 -0700, Florian Fainelli wrote: > Delay the update of the TDMA producer index unless this is the last SKB in a > batch, or the queue is already stopped. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index 9b3b8a3db59b..8419826da6a7 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -1316,8 +1316,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev) > ring->prod_index += nr_frags + 1; > ring->prod_index &= DMA_P_INDEX_MASK; > > - bcmgenet_tdma_ring_writel(priv, ring->index, > - ring->prod_index, TDMA_PROD_INDEX); > + if (!skb->xmit_more || netif_xmit_stopped(txq)) > + /* Packets are ready, update producer index */ > + bcmgenet_tdma_ring_writel(priv, ring->index, > + ring->prod_index, TDMA_PROD_INDEX); > > if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) > netif_tx_stop_queue(txq); It seems you have a race here, if you stop the queue right now, while this skb had skb->xmit_more set. -- 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
2015-03-09 15:10 GMT-07:00 Eric Dumazet <eric.dumazet@gmail.com>: > On Mon, 2015-03-09 at 14:52 -0700, Florian Fainelli wrote: >> Delay the update of the TDMA producer index unless this is the last SKB in a >> batch, or the queue is already stopped. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> index 9b3b8a3db59b..8419826da6a7 100644 >> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> @@ -1316,8 +1316,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev) >> ring->prod_index += nr_frags + 1; >> ring->prod_index &= DMA_P_INDEX_MASK; >> >> - bcmgenet_tdma_ring_writel(priv, ring->index, >> - ring->prod_index, TDMA_PROD_INDEX); >> + if (!skb->xmit_more || netif_xmit_stopped(txq)) >> + /* Packets are ready, update producer index */ >> + bcmgenet_tdma_ring_writel(priv, ring->index, >> + ring->prod_index, TDMA_PROD_INDEX); >> >> if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) >> netif_tx_stop_queue(txq); > > It seems you have a race here, if you stop the queue right now, while > this skb had skb->xmit_more set. bcmgenet_xmit() and bcmgenet_tx_reclaim() are the only two functions that can stop a queue, and they are using a spinlock to avoid that. -- Florian -- 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, 2015-03-09 at 15:13 -0700, Florian Fainelli wrote: > bcmgenet_xmit() and bcmgenet_tx_reclaim() are the only two functions > that can stop a queue, and they are using a spinlock to avoid that. It does not matter. If you hit the condition : if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) netif_tx_stop_queue(txq); Then maybe you filled the ring buffer with skb having xmit_more set, and you never did a bcmgenet_tdma_ring_writel() Look at other drivers using xmit_more, because you have to reverse things. The skeleton is : maybe_stop_tx(); if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) { mmiiowb(); ... } -- 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
2015-03-09 15:36 GMT-07:00 Eric Dumazet <eric.dumazet@gmail.com>: > On Mon, 2015-03-09 at 15:13 -0700, Florian Fainelli wrote: > >> bcmgenet_xmit() and bcmgenet_tx_reclaim() are the only two functions >> that can stop a queue, and they are using a spinlock to avoid that. > > It does not matter. > > If you hit the condition : > > if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) > netif_tx_stop_queue(txq); > > Then maybe you filled the ring buffer with skb having xmit_more set, and > you never did a bcmgenet_tdma_ring_writel() Right, thanks for pointing that out. > > Look at other drivers using xmit_more, because you have to reverse > things. > > The skeleton is : > > maybe_stop_tx(); > > if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) { > mmiiowb(); > ... > } > Will fix that, thanks
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 9b3b8a3db59b..8419826da6a7 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1316,8 +1316,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev) ring->prod_index += nr_frags + 1; ring->prod_index &= DMA_P_INDEX_MASK; - bcmgenet_tdma_ring_writel(priv, ring->index, - ring->prod_index, TDMA_PROD_INDEX); + if (!skb->xmit_more || netif_xmit_stopped(txq)) + /* Packets are ready, update producer index */ + bcmgenet_tdma_ring_writel(priv, ring->index, + ring->prod_index, TDMA_PROD_INDEX); if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) netif_tx_stop_queue(txq);
Delay the update of the TDMA producer index unless this is the last SKB in a batch, or the queue is already stopped. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)