* [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full @ 2018-12-14 8:59 Ilias Apalodimas 2018-12-14 8:59 ` [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx Ilias Apalodimas 2018-12-15 21:15 ` [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full David Miller 0 siblings, 2 replies; 9+ messages in thread From: Ilias Apalodimas @ 2018-12-14 8:59 UTC (permalink / raw) To: netdev, jaswinder.singh, ard.biesheuvel Cc: masami.hiramatsu, masahisa.kojima, Ilias Apalodimas Running pktgen with packets sizes > 512b ends up in the interface Txq getting stuck. "netsec 522d0000.ethernet eth0: netsec_netdev_start_xmit: TxQFull!" appears on dmesg but the interface never recovers. It requires an ifconfig down/up to make the interface usable again. The reason that triggers this, is a race condition between .ndo_start_xmit and the napi completion. The available budget is calculated first and indicates the queue is full. Due to a costly netif_err() the queue is not stopped in time while the napi completion runs, clears the irq and frees up descriptors, thus the queue never wakes up again. Fix this by moving the print after stopping the queue, make the print ratelimited, add barriers and check for cleaned descriptors.. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- drivers/net/ethernet/socionext/netsec.c | 56 ++++++++++++++++++++----- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c index bba9733b5119..584a6b3f6542 100644 --- a/drivers/net/ethernet/socionext/netsec.c +++ b/drivers/net/ethernet/socionext/netsec.c @@ -656,8 +656,13 @@ static int netsec_process_tx(struct netsec_priv *priv, int budget) budget -= new; } while (new); - if (done && netif_queue_stopped(ndev)) + if (done && netif_queue_stopped(ndev)) { + /* Make sure we update the value, anyone stopping the queue + * after this will read the proper consumer idx + */ + smp_wmb(); netif_wake_queue(ndev); + } return done; } @@ -877,6 +882,41 @@ static void netsec_set_tx_de(struct netsec_priv *priv, dring->head = (dring->head + 1) % DESC_NUM; } +static int netsec_desc_used(struct netsec_desc_ring *dring) +{ + int used; + + if (dring->head >= dring->tail) + used = dring->head - dring->tail; + else + used = dring->head + DESC_NUM - dring->tail; + + return used; +} + +static int netsec_check_stop_tx(struct netsec_priv *priv, int used) +{ + struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX]; + + /* keep tail from touching the queue */ + if (DESC_NUM - used < 2) { + netif_stop_queue(priv->ndev); + + /* Make sure we read the updated value in case + * descriptors got freed + */ + smp_rmb(); + + used = netsec_desc_used(dring); + if (DESC_NUM - used < 2) + return NETDEV_TX_BUSY; + + netif_wake_queue(priv->ndev); + } + + return 0; +} + static netdev_tx_t netsec_netdev_start_xmit(struct sk_buff *skb, struct net_device *ndev) { @@ -887,16 +927,10 @@ static netdev_tx_t netsec_netdev_start_xmit(struct sk_buff *skb, u16 tso_seg_len = 0; int filled; - /* differentiate between full/emtpy ring */ - if (dring->head >= dring->tail) - filled = dring->head - dring->tail; - else - filled = dring->head + DESC_NUM - dring->tail; - - if (DESC_NUM - filled < 2) { /* if less than 2 available */ - netif_err(priv, drv, priv->ndev, "%s: TxQFull!\n", __func__); - netif_stop_queue(priv->ndev); - dma_wmb(); + filled = netsec_desc_used(dring); + if (netsec_check_stop_tx(priv, filled)) { + net_warn_ratelimited("%s %s Tx queue full\n", + dev_name(priv->dev), ndev->name); return NETDEV_TX_BUSY; } -- 2.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 8:59 [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full Ilias Apalodimas @ 2018-12-14 8:59 ` Ilias Apalodimas 2018-12-14 10:59 ` Ard Biesheuvel 2018-12-15 21:15 ` David Miller 2018-12-15 21:15 ` [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full David Miller 1 sibling, 2 replies; 9+ messages in thread From: Ilias Apalodimas @ 2018-12-14 8:59 UTC (permalink / raw) To: netdev, jaswinder.singh, ard.biesheuvel Cc: masami.hiramatsu, masahisa.kojima, Ilias Apalodimas Currently the driver issues 2 mmio reads to figure out the number of transmitted packets and clean them. We can get rid of the expensive reads since BIT 31 of the Tx descriptor can be used for that. We can also remove the budget counting of Tx completions since all of the descriptors are not deliberately processed. Performance numbers using pktgen are: size pre-patch(pps) post-patch(pps) 64 362483 427916 128 358315 411686 256 352725 389683 512 215675 216464 1024 113812 114442 Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- drivers/net/ethernet/socionext/netsec.c | 97 ++++++++++++++----------- 1 file changed, 54 insertions(+), 43 deletions(-) diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c index 584a6b3f6542..05a0948ad929 100644 --- a/drivers/net/ethernet/socionext/netsec.c +++ b/drivers/net/ethernet/socionext/netsec.c @@ -257,7 +257,6 @@ struct netsec_desc_ring { dma_addr_t desc_dma; struct netsec_desc *desc; void *vaddr; - u16 pkt_cnt; u16 head, tail; }; @@ -598,33 +597,26 @@ static void netsec_set_rx_de(struct netsec_priv *priv, dring->desc[idx].len = desc->len; } -static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) +static bool netsec_clean_tx_dring(struct netsec_priv *priv) { struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX]; unsigned int pkts, bytes; - - dring->pkt_cnt += netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); - - if (dring->pkt_cnt < budget) - budget = dring->pkt_cnt; + struct netsec_de *entry; + int tail = dring->tail; + int cnt = 0; pkts = 0; bytes = 0; + entry = dring->vaddr + DESC_SZ * tail; - while (pkts < budget) { + while (!(entry->attr & (1U << NETSEC_TX_SHIFT_OWN_FIELD)) && + cnt < DESC_NUM) { struct netsec_desc *desc; - struct netsec_de *entry; - int tail, eop; - - tail = dring->tail; - - /* move tail ahead */ - dring->tail = (tail + 1) % DESC_NUM; + int eop; desc = &dring->desc[tail]; - entry = dring->vaddr + DESC_SZ * tail; - eop = (entry->attr >> NETSEC_TX_LAST) & 1; + dma_rmb(); dma_unmap_single(priv->dev, desc->dma_addr, desc->len, DMA_TO_DEVICE); @@ -633,38 +625,51 @@ static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) bytes += desc->skb->len; dev_kfree_skb(desc->skb); } + /* clean up so netsec_uninit_pkt_dring() won't free the skb + * again + */ *desc = (struct netsec_desc){}; + + /* entry->attr is not going to be accessed by the NIC until + * netsec_set_tx_de() is called. No need for a dma_wmb() here + */ + entry->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; + /* move tail ahead */ + dring->tail = (tail + 1) % DESC_NUM; + + tail = dring->tail; + entry = dring->vaddr + DESC_SZ * tail; + cnt++; } - dring->pkt_cnt -= budget; - priv->ndev->stats.tx_packets += budget; + if (!cnt) + return false; + + /* reading the register clears the irq */ + netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); + + priv->ndev->stats.tx_packets += cnt; priv->ndev->stats.tx_bytes += bytes; - netdev_completed_queue(priv->ndev, budget, bytes); + netdev_completed_queue(priv->ndev, cnt, bytes); - return budget; + return true; } -static int netsec_process_tx(struct netsec_priv *priv, int budget) +static void netsec_process_tx(struct netsec_priv *priv) { struct net_device *ndev = priv->ndev; - int new, done = 0; + bool cleaned; - do { - new = netsec_clean_tx_dring(priv, budget); - done += new; - budget -= new; - } while (new); + cleaned = netsec_clean_tx_dring(priv); - if (done && netif_queue_stopped(ndev)) { + if (cleaned && netif_queue_stopped(ndev)) { /* Make sure we update the value, anyone stopping the queue * after this will read the proper consumer idx */ smp_wmb(); netif_wake_queue(ndev); } - - return done; } static void *netsec_alloc_rx_data(struct netsec_priv *priv, @@ -813,24 +818,17 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) static int netsec_napi_poll(struct napi_struct *napi, int budget) { struct netsec_priv *priv; - int tx, rx, done, todo; + int rx, done, todo; priv = container_of(napi, struct netsec_priv, napi); + netsec_process_tx(priv); + todo = budget; do { - if (!todo) - break; - - tx = netsec_process_tx(priv, todo); - todo -= tx; - - if (!todo) - break; - rx = netsec_process_rx(priv, todo); todo -= rx; - } while (rx || tx); + } while (rx); done = budget - todo; @@ -1007,7 +1005,6 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id) dring->head = 0; dring->tail = 0; - dring->pkt_cnt = 0; if (id == NETSEC_RING_TX) netdev_reset_queue(priv->ndev); @@ -1030,6 +1027,7 @@ static void netsec_free_dring(struct netsec_priv *priv, int id) static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) { struct netsec_desc_ring *dring = &priv->desc_ring[id]; + int i; dring->vaddr = dma_zalloc_coherent(priv->dev, DESC_SZ * DESC_NUM, &dring->desc_dma, GFP_KERNEL); @@ -1040,6 +1038,19 @@ static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) if (!dring->desc) goto err; + if (id == NETSEC_RING_TX) { + for (i = 0; i < DESC_NUM; i++) { + struct netsec_de *de; + + de = dring->vaddr + (DESC_SZ * i); + /* de->attr is not going to be accessed by the NIC + * until netsec_set_tx_de() is called. + * No need for a dma_wmb() here + */ + de->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; + } + } + return 0; err: netsec_free_dring(priv, id); -- 2.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 8:59 ` [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx Ilias Apalodimas @ 2018-12-14 10:59 ` Ard Biesheuvel 2018-12-14 11:18 ` Ilias Apalodimas 2018-12-15 21:15 ` David Miller 1 sibling, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2018-12-14 10:59 UTC (permalink / raw) To: Ilias Apalodimas Cc: <netdev@vger.kernel.org>, Jaswinder Singh, Masami Hiramatsu, Masahisa Kojima On Fri, 14 Dec 2018 at 09:59, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Currently the driver issues 2 mmio reads to figure out the number of > transmitted packets and clean them. We can get rid of the expensive > reads since BIT 31 of the Tx descriptor can be used for that. > We can also remove the budget counting of Tx completions since all of > the descriptors are not deliberately processed. > This last sentence reflects what we discussed off-line, right? That reaping used Tx descriptors shouldn't be counted against the budget, but should only occur if the budget is > 0 after processing the Rx queue? </ilias.apalodimas@linaro.org> > Performance numbers using pktgen are: > size pre-patch(pps) post-patch(pps) > 64 362483 427916 > 128 358315 411686 > 256 352725 389683 > 512 215675 216464 > 1024 113812 114442 > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > drivers/net/ethernet/socionext/netsec.c | 97 ++++++++++++++----------- > 1 file changed, 54 insertions(+), 43 deletions(-) > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > index 584a6b3f6542..05a0948ad929 100644 > --- a/drivers/net/ethernet/socionext/netsec.c > +++ b/drivers/net/ethernet/socionext/netsec.c > @@ -257,7 +257,6 @@ struct netsec_desc_ring { > dma_addr_t desc_dma; > struct netsec_desc *desc; > void *vaddr; > - u16 pkt_cnt; > u16 head, tail; > }; > > @@ -598,33 +597,26 @@ static void netsec_set_rx_de(struct netsec_priv *priv, > dring->desc[idx].len = desc->len; > } > > -static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > +static bool netsec_clean_tx_dring(struct netsec_priv *priv) > { > struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX]; > unsigned int pkts, bytes; > - > - dring->pkt_cnt += netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > - > - if (dring->pkt_cnt < budget) > - budget = dring->pkt_cnt; > + struct netsec_de *entry; > + int tail = dring->tail; > + int cnt = 0; > > pkts = 0; > bytes = 0; > + entry = dring->vaddr + DESC_SZ * tail; > > - while (pkts < budget) { > + while (!(entry->attr & (1U << NETSEC_TX_SHIFT_OWN_FIELD)) && > + cnt < DESC_NUM) { > struct netsec_desc *desc; > - struct netsec_de *entry; > - int tail, eop; > - > - tail = dring->tail; > - > - /* move tail ahead */ > - dring->tail = (tail + 1) % DESC_NUM; > + int eop; > > desc = &dring->desc[tail]; > - entry = dring->vaddr + DESC_SZ * tail; > - > eop = (entry->attr >> NETSEC_TX_LAST) & 1; > + dma_rmb(); > > dma_unmap_single(priv->dev, desc->dma_addr, desc->len, > DMA_TO_DEVICE); > @@ -633,38 +625,51 @@ static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > bytes += desc->skb->len; > dev_kfree_skb(desc->skb); > } > + /* clean up so netsec_uninit_pkt_dring() won't free the skb > + * again > + */ > *desc = (struct netsec_desc){}; > + > + /* entry->attr is not going to be accessed by the NIC until > + * netsec_set_tx_de() is called. No need for a dma_wmb() here > + */ > + entry->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; > + /* move tail ahead */ > + dring->tail = (tail + 1) % DESC_NUM; > + > + tail = dring->tail; > + entry = dring->vaddr + DESC_SZ * tail; > + cnt++; > } > - dring->pkt_cnt -= budget; > > - priv->ndev->stats.tx_packets += budget; > + if (!cnt) > + return false; > + > + /* reading the register clears the irq */ > + netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > + > + priv->ndev->stats.tx_packets += cnt; > priv->ndev->stats.tx_bytes += bytes; > > - netdev_completed_queue(priv->ndev, budget, bytes); > + netdev_completed_queue(priv->ndev, cnt, bytes); > > - return budget; > + return true; > } > > -static int netsec_process_tx(struct netsec_priv *priv, int budget) > +static void netsec_process_tx(struct netsec_priv *priv) > { > struct net_device *ndev = priv->ndev; > - int new, done = 0; > + bool cleaned; > > - do { > - new = netsec_clean_tx_dring(priv, budget); > - done += new; > - budget -= new; > - } while (new); > + cleaned = netsec_clean_tx_dring(priv); > > - if (done && netif_queue_stopped(ndev)) { > + if (cleaned && netif_queue_stopped(ndev)) { > /* Make sure we update the value, anyone stopping the queue > * after this will read the proper consumer idx > */ > smp_wmb(); > netif_wake_queue(ndev); > } > - > - return done; > } > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > @@ -813,24 +818,17 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > static int netsec_napi_poll(struct napi_struct *napi, int budget) > { > struct netsec_priv *priv; > - int tx, rx, done, todo; > + int rx, done, todo; > > priv = container_of(napi, struct netsec_priv, napi); > > + netsec_process_tx(priv); > + So shouldn't we do this *after* processing Rx, and only if there is budget left? > todo = budget; > do { > - if (!todo) > - break; > - > - tx = netsec_process_tx(priv, todo); > - todo -= tx; > - > - if (!todo) > - break; > - > rx = netsec_process_rx(priv, todo); > todo -= rx; > - } while (rx || tx); > + } while (rx); > > done = budget - todo; > > @@ -1007,7 +1005,6 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id) > > dring->head = 0; > dring->tail = 0; > - dring->pkt_cnt = 0; > > if (id == NETSEC_RING_TX) > netdev_reset_queue(priv->ndev); > @@ -1030,6 +1027,7 @@ static void netsec_free_dring(struct netsec_priv *priv, int id) > static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) > { > struct netsec_desc_ring *dring = &priv->desc_ring[id]; > + int i; > > dring->vaddr = dma_zalloc_coherent(priv->dev, DESC_SZ * DESC_NUM, > &dring->desc_dma, GFP_KERNEL); > @@ -1040,6 +1038,19 @@ static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) > if (!dring->desc) > goto err; > > + if (id == NETSEC_RING_TX) { > + for (i = 0; i < DESC_NUM; i++) { > + struct netsec_de *de; > + > + de = dring->vaddr + (DESC_SZ * i); > + /* de->attr is not going to be accessed by the NIC > + * until netsec_set_tx_de() is called. > + * No need for a dma_wmb() here > + */ > + de->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; > + } > + } > + > return 0; > err: > netsec_free_dring(priv, id); > -- > 2.19.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 10:59 ` Ard Biesheuvel @ 2018-12-14 11:18 ` Ilias Apalodimas 2018-12-14 14:16 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Ilias Apalodimas @ 2018-12-14 11:18 UTC (permalink / raw) To: Ard Biesheuvel Cc: <netdev@vger.kernel.org>, Jaswinder Singh, Masami Hiramatsu, Masahisa Kojima On Fri, Dec 14, 2018 at 11:59:00AM +0100, Ard Biesheuvel wrote: > On Fri, 14 Dec 2018 at 09:59, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Currently the driver issues 2 mmio reads to figure out the number of > > transmitted packets and clean them. We can get rid of the expensive > > reads since BIT 31 of the Tx descriptor can be used for that. > > We can also remove the budget counting of Tx completions since all of > > the descriptors are not deliberately processed. > > > > This last sentence reflects what we discussed off-line, right? That > reaping used Tx descriptors shouldn't be counted against the budget, > but should only occur if the budget is > 0 after processing the Rx > queue? yes > > Performance numbers using pktgen are: > > size pre-patch(pps) post-patch(pps) > > 64 362483 427916 > > 128 358315 411686 > > 256 352725 389683 > > 512 215675 216464 > > 1024 113812 114442 > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > drivers/net/ethernet/socionext/netsec.c | 97 ++++++++++++++----------- > > 1 file changed, 54 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > > index 584a6b3f6542..05a0948ad929 100644 > > --- a/drivers/net/ethernet/socionext/netsec.c > > +++ b/drivers/net/ethernet/socionext/netsec.c > > @@ -257,7 +257,6 @@ struct netsec_desc_ring { > > dma_addr_t desc_dma; > > struct netsec_desc *desc; > > void *vaddr; > > - u16 pkt_cnt; > > u16 head, tail; > > }; > > > > @@ -598,33 +597,26 @@ static void netsec_set_rx_de(struct netsec_priv *priv, > > dring->desc[idx].len = desc->len; > > } > > > > -static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > > +static bool netsec_clean_tx_dring(struct netsec_priv *priv) > > { > > struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX]; > > unsigned int pkts, bytes; > > - > > - dring->pkt_cnt += netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > > - > > - if (dring->pkt_cnt < budget) > > - budget = dring->pkt_cnt; > > + struct netsec_de *entry; > > + int tail = dring->tail; > > + int cnt = 0; > > > > pkts = 0; > > bytes = 0; > > + entry = dring->vaddr + DESC_SZ * tail; > > > > - while (pkts < budget) { > > + while (!(entry->attr & (1U << NETSEC_TX_SHIFT_OWN_FIELD)) && > > + cnt < DESC_NUM) { > > struct netsec_desc *desc; > > - struct netsec_de *entry; > > - int tail, eop; > > - > > - tail = dring->tail; > > - > > - /* move tail ahead */ > > - dring->tail = (tail + 1) % DESC_NUM; > > + int eop; > > > > desc = &dring->desc[tail]; > > - entry = dring->vaddr + DESC_SZ * tail; > > - > > eop = (entry->attr >> NETSEC_TX_LAST) & 1; > > + dma_rmb(); > > > > dma_unmap_single(priv->dev, desc->dma_addr, desc->len, > > DMA_TO_DEVICE); > > @@ -633,38 +625,51 @@ static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > > bytes += desc->skb->len; > > dev_kfree_skb(desc->skb); > > } > > + /* clean up so netsec_uninit_pkt_dring() won't free the skb > > + * again > > + */ > > *desc = (struct netsec_desc){}; > > + > > + /* entry->attr is not going to be accessed by the NIC until > > + * netsec_set_tx_de() is called. No need for a dma_wmb() here > > + */ > > + entry->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; > > + /* move tail ahead */ > > + dring->tail = (tail + 1) % DESC_NUM; > > + > > + tail = dring->tail; > > + entry = dring->vaddr + DESC_SZ * tail; > > + cnt++; > > } > > - dring->pkt_cnt -= budget; > > > > - priv->ndev->stats.tx_packets += budget; > > + if (!cnt) > > + return false; > > + > > + /* reading the register clears the irq */ > > + netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > > + > > + priv->ndev->stats.tx_packets += cnt; > > priv->ndev->stats.tx_bytes += bytes; > > > > - netdev_completed_queue(priv->ndev, budget, bytes); > > + netdev_completed_queue(priv->ndev, cnt, bytes); > > > > - return budget; > > + return true; > > } > > > > -static int netsec_process_tx(struct netsec_priv *priv, int budget) > > +static void netsec_process_tx(struct netsec_priv *priv) > > { > > struct net_device *ndev = priv->ndev; > > - int new, done = 0; > > + bool cleaned; > > > > - do { > > - new = netsec_clean_tx_dring(priv, budget); > > - done += new; > > - budget -= new; > > - } while (new); > > + cleaned = netsec_clean_tx_dring(priv); > > > > - if (done && netif_queue_stopped(ndev)) { > > + if (cleaned && netif_queue_stopped(ndev)) { > > /* Make sure we update the value, anyone stopping the queue > > * after this will read the proper consumer idx > > */ > > smp_wmb(); > > netif_wake_queue(ndev); > > } > > - > > - return done; > > } > > > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > @@ -813,24 +818,17 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > static int netsec_napi_poll(struct napi_struct *napi, int budget) > > { > > struct netsec_priv *priv; > > - int tx, rx, done, todo; > > + int rx, done, todo; > > > > priv = container_of(napi, struct netsec_priv, napi); > > > > + netsec_process_tx(priv); > > + > > So shouldn't we do this *after* processing Rx, and only if there is budget left? I am not really sure, this would drown Tx processing if you had a bunch of received packets that exhausted the budget. Intel 1gbit drivers are doing something similar. They reclaim Tx prior to processing Rx. The only thing that could be checked here i guess is keep the napi poll running if *all* Tx descriptors were processed at some point instead of re-enabling irqs. > > > todo = budget; > > do { > > - if (!todo) > > - break; > > - > > - tx = netsec_process_tx(priv, todo); > > - todo -= tx; > > - > > - if (!todo) > > - break; > > - > > rx = netsec_process_rx(priv, todo); > > todo -= rx; > > - } while (rx || tx); > > + } while (rx); > > > > done = budget - todo; > > > > @@ -1007,7 +1005,6 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id) > > > > dring->head = 0; > > dring->tail = 0; > > - dring->pkt_cnt = 0; > > > > if (id == NETSEC_RING_TX) > > netdev_reset_queue(priv->ndev); > > @@ -1030,6 +1027,7 @@ static void netsec_free_dring(struct netsec_priv *priv, int id) > > static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) > > { > > struct netsec_desc_ring *dring = &priv->desc_ring[id]; > > + int i; > > > > dring->vaddr = dma_zalloc_coherent(priv->dev, DESC_SZ * DESC_NUM, > > &dring->desc_dma, GFP_KERNEL); > > @@ -1040,6 +1038,19 @@ static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id) > > if (!dring->desc) > > goto err; > > > > + if (id == NETSEC_RING_TX) { > > + for (i = 0; i < DESC_NUM; i++) { > > + struct netsec_de *de; > > + > > + de = dring->vaddr + (DESC_SZ * i); > > + /* de->attr is not going to be accessed by the NIC > > + * until netsec_set_tx_de() is called. > > + * No need for a dma_wmb() here > > + */ > > + de->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; > > + } > > + } > > + > > return 0; > > err: > > netsec_free_dring(priv, id); > > -- > > 2.19.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 11:18 ` Ilias Apalodimas @ 2018-12-14 14:16 ` Ard Biesheuvel 2018-12-14 14:32 ` Ilias Apalodimas 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2018-12-14 14:16 UTC (permalink / raw) To: Ilias Apalodimas Cc: <netdev@vger.kernel.org>, Jaswinder Singh, Masami Hiramatsu, Masahisa Kojima On Fri, 14 Dec 2018 at 12:18, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Fri, Dec 14, 2018 at 11:59:00AM +0100, Ard Biesheuvel wrote: > > On Fri, 14 Dec 2018 at 09:59, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Currently the driver issues 2 mmio reads to figure out the number of > > > transmitted packets and clean them. We can get rid of the expensive > > > reads since BIT 31 of the Tx descriptor can be used for that. > > > We can also remove the budget counting of Tx completions since all of > > > the descriptors are not deliberately processed. > > > > > > > This last sentence reflects what we discussed off-line, right? That > > reaping used Tx descriptors shouldn't be counted against the budget, > > but should only occur if the budget is > 0 after processing the Rx > > queue? > > yes > > > > Performance numbers using pktgen are: > > > size pre-patch(pps) post-patch(pps) > > > 64 362483 427916 > > > 128 358315 411686 > > > 256 352725 389683 > > > 512 215675 216464 > > > 1024 113812 114442 > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > drivers/net/ethernet/socionext/netsec.c | 97 ++++++++++++++----------- > > > 1 file changed, 54 insertions(+), 43 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > > > index 584a6b3f6542..05a0948ad929 100644 > > > --- a/drivers/net/ethernet/socionext/netsec.c > > > +++ b/drivers/net/ethernet/socionext/netsec.c > > > @@ -257,7 +257,6 @@ struct netsec_desc_ring { > > > dma_addr_t desc_dma; > > > struct netsec_desc *desc; > > > void *vaddr; > > > - u16 pkt_cnt; > > > u16 head, tail; > > > }; > > > > > > @@ -598,33 +597,26 @@ static void netsec_set_rx_de(struct netsec_priv *priv, > > > dring->desc[idx].len = desc->len; > > > } > > > > > > -static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > > > +static bool netsec_clean_tx_dring(struct netsec_priv *priv) > > > { > > > struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX]; > > > unsigned int pkts, bytes; > > > - > > > - dring->pkt_cnt += netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > > > - > > > - if (dring->pkt_cnt < budget) > > > - budget = dring->pkt_cnt; > > > + struct netsec_de *entry; > > > + int tail = dring->tail; > > > + int cnt = 0; > > > > > > pkts = 0; > > > bytes = 0; > > > + entry = dring->vaddr + DESC_SZ * tail; > > > > > > - while (pkts < budget) { > > > + while (!(entry->attr & (1U << NETSEC_TX_SHIFT_OWN_FIELD)) && > > > + cnt < DESC_NUM) { > > > struct netsec_desc *desc; > > > - struct netsec_de *entry; > > > - int tail, eop; > > > - > > > - tail = dring->tail; > > > - > > > - /* move tail ahead */ > > > - dring->tail = (tail + 1) % DESC_NUM; > > > + int eop; > > > > > > desc = &dring->desc[tail]; > > > - entry = dring->vaddr + DESC_SZ * tail; > > > - > > > eop = (entry->attr >> NETSEC_TX_LAST) & 1; > > > + dma_rmb(); > > > > > > dma_unmap_single(priv->dev, desc->dma_addr, desc->len, > > > DMA_TO_DEVICE); > > > @@ -633,38 +625,51 @@ static int netsec_clean_tx_dring(struct netsec_priv *priv, int budget) > > > bytes += desc->skb->len; > > > dev_kfree_skb(desc->skb); > > > } > > > + /* clean up so netsec_uninit_pkt_dring() won't free the skb > > > + * again > > > + */ > > > *desc = (struct netsec_desc){}; > > > + > > > + /* entry->attr is not going to be accessed by the NIC until > > > + * netsec_set_tx_de() is called. No need for a dma_wmb() here > > > + */ > > > + entry->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD; > > > + /* move tail ahead */ > > > + dring->tail = (tail + 1) % DESC_NUM; > > > + > > > + tail = dring->tail; > > > + entry = dring->vaddr + DESC_SZ * tail; > > > + cnt++; > > > } > > > - dring->pkt_cnt -= budget; > > > > > > - priv->ndev->stats.tx_packets += budget; > > > + if (!cnt) > > > + return false; > > > + > > > + /* reading the register clears the irq */ > > > + netsec_read(priv, NETSEC_REG_NRM_TX_DONE_PKTCNT); > > > + > > > + priv->ndev->stats.tx_packets += cnt; > > > priv->ndev->stats.tx_bytes += bytes; > > > > > > - netdev_completed_queue(priv->ndev, budget, bytes); > > > + netdev_completed_queue(priv->ndev, cnt, bytes); > > > > > > - return budget; > > > + return true; > > > } > > > > > > -static int netsec_process_tx(struct netsec_priv *priv, int budget) > > > +static void netsec_process_tx(struct netsec_priv *priv) > > > { > > > struct net_device *ndev = priv->ndev; > > > - int new, done = 0; > > > + bool cleaned; > > > > > > - do { > > > - new = netsec_clean_tx_dring(priv, budget); > > > - done += new; > > > - budget -= new; > > > - } while (new); > > > + cleaned = netsec_clean_tx_dring(priv); > > > > > > - if (done && netif_queue_stopped(ndev)) { > > > + if (cleaned && netif_queue_stopped(ndev)) { > > > /* Make sure we update the value, anyone stopping the queue > > > * after this will read the proper consumer idx > > > */ > > > smp_wmb(); > > > netif_wake_queue(ndev); > > > } > > > - > > > - return done; > > > } > > > > > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > > @@ -813,24 +818,17 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > > static int netsec_napi_poll(struct napi_struct *napi, int budget) > > > { > > > struct netsec_priv *priv; > > > - int tx, rx, done, todo; > > > + int rx, done, todo; > > > > > > priv = container_of(napi, struct netsec_priv, napi); > > > > > > + netsec_process_tx(priv); > > > + > > > > So shouldn't we do this *after* processing Rx, and only if there is budget left? > > I am not really sure, this would drown Tx processing if you had a bunch of > received packets that exhausted the budget. > Intel 1gbit drivers are doing something similar. They reclaim Tx prior to > processing Rx. The only thing that could be checked here i guess is keep the > napi poll running if *all* Tx descriptors were processed at some point instead > of re-enabling irqs. > The reason I suggest it is because you quoted it from the documentation :-) But if reality deviates from that, then sure, let's follow the examples of others. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 14:16 ` Ard Biesheuvel @ 2018-12-14 14:32 ` Ilias Apalodimas 2018-12-14 20:05 ` Jassi Brar 0 siblings, 1 reply; 9+ messages in thread From: Ilias Apalodimas @ 2018-12-14 14:32 UTC (permalink / raw) To: Ard Biesheuvel Cc: <netdev@vger.kernel.org>, Jaswinder Singh, Masami Hiramatsu, Masahisa Kojima Hi Ard, > > > > > > So shouldn't we do this *after* processing Rx, and only if there is budget left? > > > > I am not really sure, this would drown Tx processing if you had a bunch of > > received packets that exhausted the budget. > > Intel 1gbit drivers are doing something similar. They reclaim Tx prior to > > processing Rx. The only thing that could be checked here i guess is keep the > > napi poll running if *all* Tx descriptors were processed at some point instead > > of re-enabling irqs. > > > > The reason I suggest it is because you quoted it from the documentation :-) Yes i understand. I had my doubts as well. That's hy i tried following the example of another driver. > > But if reality deviates from that, then sure, let's follow the > examples of others. Agree. Unless someone has any objections i am fine with the current patches. Thanks! /Ilias ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 14:32 ` Ilias Apalodimas @ 2018-12-14 20:05 ` Jassi Brar 0 siblings, 0 replies; 9+ messages in thread From: Jassi Brar @ 2018-12-14 20:05 UTC (permalink / raw) To: Ilias Apalodimas Cc: Ard Biesheuvel, <netdev@vger.kernel.org>, Masami Hiramatsu, Masahisa Kojima On Fri, 14 Dec 2018 at 08:33, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Ard, > > > > > > > > So shouldn't we do this *after* processing Rx, and only if there is budget left? > > > > > > I am not really sure, this would drown Tx processing if you had a bunch of > > > received packets that exhausted the budget. > > > Intel 1gbit drivers are doing something similar. They reclaim Tx prior to > > > processing Rx. The only thing that could be checked here i guess is keep the > > > napi poll running if *all* Tx descriptors were processed at some point instead > > > of re-enabling irqs. > > > > > > > The reason I suggest it is because you quoted it from the documentation :-) > > Yes i understand. I had my doubts as well. > That's hy i tried following the example of another driver. > > > > > But if reality deviates from that, then sure, let's follow the > > examples of others. > > Agree. Unless someone has any objections i am fine with the current patches. > The idea was to do as much as we can in one irq and irqs disabled for most of the time. But the numbers are more important, so I am fine too. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx 2018-12-14 8:59 ` [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx Ilias Apalodimas 2018-12-14 10:59 ` Ard Biesheuvel @ 2018-12-15 21:15 ` David Miller 1 sibling, 0 replies; 9+ messages in thread From: David Miller @ 2018-12-15 21:15 UTC (permalink / raw) To: ilias.apalodimas Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, masahisa.kojima From: Ilias Apalodimas <ilias.apalodimas@linaro.org> Date: Fri, 14 Dec 2018 10:59:01 +0200 > Currently the driver issues 2 mmio reads to figure out the number of > transmitted packets and clean them. We can get rid of the expensive > reads since BIT 31 of the Tx descriptor can be used for that. > We can also remove the budget counting of Tx completions since all of > the descriptors are not deliberately processed. > > Performance numbers using pktgen are: > size pre-patch(pps) post-patch(pps) > 64 362483 427916 > 128 358315 411686 > 256 352725 389683 > 512 215675 216464 > 1024 113812 114442 > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Applied. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full 2018-12-14 8:59 [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full Ilias Apalodimas 2018-12-14 8:59 ` [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx Ilias Apalodimas @ 2018-12-15 21:15 ` David Miller 1 sibling, 0 replies; 9+ messages in thread From: David Miller @ 2018-12-15 21:15 UTC (permalink / raw) To: ilias.apalodimas Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, masahisa.kojima From: Ilias Apalodimas <ilias.apalodimas@linaro.org> Date: Fri, 14 Dec 2018 10:59:00 +0200 > Running pktgen with packets sizes > 512b ends up in the interface Txq > getting stuck. > "netsec 522d0000.ethernet eth0: netsec_netdev_start_xmit: TxQFull!" > appears on dmesg but the interface never recovers. It requires an > ifconfig down/up to make the interface usable again. > > The reason that triggers this, is a race condition between > .ndo_start_xmit and the napi completion. The available budget is > calculated first and indicates the queue is full. Due to a costly > netif_err() the queue is not stopped in time while the napi completion > runs, clears the irq and frees up descriptors, thus the queue never wakes > up again. > > Fix this by moving the print after stopping the queue, make the print > ratelimited, add barriers and check for cleaned descriptors.. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Applied. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-15 21:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-14 8:59 [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full Ilias Apalodimas 2018-12-14 8:59 ` [net-next, PATCH 2/2] net: socionext: remove mmio reads on Tx Ilias Apalodimas 2018-12-14 10:59 ` Ard Biesheuvel 2018-12-14 11:18 ` Ilias Apalodimas 2018-12-14 14:16 ` Ard Biesheuvel 2018-12-14 14:32 ` Ilias Apalodimas 2018-12-14 20:05 ` Jassi Brar 2018-12-15 21:15 ` David Miller 2018-12-15 21:15 ` [net-next, PATCH 1/2] net: socionext: correctly recover txq after being full 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).