| Message ID | 20250214211643.2617340-1-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: cadence: macb: Implement BQL | expand |
On Fri, 14 Feb 2025 16:16:43 -0500 Sean Anderson wrote: > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 48496209fb16..63c65b4bb348 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -1081,6 +1081,9 @@ static void macb_tx_error_task(struct work_struct *work) > tx_error_task); > bool halt_timeout = false; > struct macb *bp = queue->bp; > + u32 queue_index = queue - bp->queues; nit: breaking reverse xmas tree here > + u32 packets = 0; > + u32 bytes = 0; > struct macb_tx_skb *tx_skb; > struct macb_dma_desc *desc; > struct sk_buff *skb; > @@ -3019,6 +3033,7 @@ static int macb_close(struct net_device *dev) > netif_tx_stop_all_queues(dev); > > for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { > + netdev_tx_reset_queue(netdev_get_tx_queue(dev, q)); > napi_disable(&queue->napi_rx); > napi_disable(&queue->napi_tx); I think you should reset after napi_disable()? Lest NAPI runs after the reset and tries to complete on an empty queue..
On 2/18/25 20:57, Jakub Kicinski wrote: > On Fri, 14 Feb 2025 16:16:43 -0500 Sean Anderson wrote: >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 48496209fb16..63c65b4bb348 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -1081,6 +1081,9 @@ static void macb_tx_error_task(struct work_struct *work) >> tx_error_task); >> bool halt_timeout = false; >> struct macb *bp = queue->bp; >> + u32 queue_index = queue - bp->queues; > > nit: breaking reverse xmas tree here It has to happen here since bp isn't available earlier. >> + u32 packets = 0; >> + u32 bytes = 0; >> struct macb_tx_skb *tx_skb; >> struct macb_dma_desc *desc; >> struct sk_buff *skb; > > >> @@ -3019,6 +3033,7 @@ static int macb_close(struct net_device *dev) >> netif_tx_stop_all_queues(dev); >> >> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { >> + netdev_tx_reset_queue(netdev_get_tx_queue(dev, q)); >> napi_disable(&queue->napi_rx); >> napi_disable(&queue->napi_tx); > > I think you should reset after napi_disable()? > Lest NAPI runs after the reset and tries to complete on an empty queue.. OK. --Sean
On Thu, 20 Feb 2025 10:55:45 -0500 Sean Anderson wrote: > On 2/18/25 20:57, Jakub Kicinski wrote: > > On Fri, 14 Feb 2025 16:16:43 -0500 Sean Anderson wrote: > >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > >> index 48496209fb16..63c65b4bb348 100644 > >> --- a/drivers/net/ethernet/cadence/macb_main.c > >> +++ b/drivers/net/ethernet/cadence/macb_main.c > >> @@ -1081,6 +1081,9 @@ static void macb_tx_error_task(struct work_struct *work) > >> tx_error_task); > >> bool halt_timeout = false; > >> struct macb *bp = queue->bp; > >> + u32 queue_index = queue - bp->queues; > > > > nit: breaking reverse xmas tree here > > It has to happen here since bp isn't available earlier. Move it from init to normal code?
On 2/20/25 11:51, Jakub Kicinski wrote: > On Thu, 20 Feb 2025 10:55:45 -0500 Sean Anderson wrote: >> On 2/18/25 20:57, Jakub Kicinski wrote: >> > On Fri, 14 Feb 2025 16:16:43 -0500 Sean Anderson wrote: >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> >> index 48496209fb16..63c65b4bb348 100644 >> >> --- a/drivers/net/ethernet/cadence/macb_main.c >> >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> >> @@ -1081,6 +1081,9 @@ static void macb_tx_error_task(struct work_struct *work) >> >> tx_error_task); >> >> bool halt_timeout = false; >> >> struct macb *bp = queue->bp; >> >> + u32 queue_index = queue - bp->queues; >> > >> > nit: breaking reverse xmas tree here >> >> It has to happen here since bp isn't available earlier. > > Move it from init to normal code? That's what I ended up doing. --Sean
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 48496209fb16..63c65b4bb348 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -1081,6 +1081,9 @@ static void macb_tx_error_task(struct work_struct *work) tx_error_task); bool halt_timeout = false; struct macb *bp = queue->bp; + u32 queue_index = queue - bp->queues; + u32 packets = 0; + u32 bytes = 0; struct macb_tx_skb *tx_skb; struct macb_dma_desc *desc; struct sk_buff *skb; @@ -1088,8 +1091,7 @@ static void macb_tx_error_task(struct work_struct *work) unsigned long flags; netdev_vdbg(bp->dev, "macb_tx_error_task: q = %u, t = %u, h = %u\n", - (unsigned int)(queue - bp->queues), - queue->tx_tail, queue->tx_head); + queue_index, queue->tx_tail, queue->tx_head); /* Prevent the queue NAPI TX poll from running, as it calls * macb_tx_complete(), which in turn may call netif_wake_subqueue(). @@ -1142,8 +1144,10 @@ static void macb_tx_error_task(struct work_struct *work) skb->data); bp->dev->stats.tx_packets++; queue->stats.tx_packets++; + packets++; bp->dev->stats.tx_bytes += skb->len; queue->stats.tx_bytes += skb->len; + bytes += skb->len; } } else { /* "Buffers exhausted mid-frame" errors may only happen @@ -1160,6 +1164,9 @@ static void macb_tx_error_task(struct work_struct *work) macb_tx_unmap(bp, tx_skb, 0); } + netdev_tx_completed_queue(netdev_get_tx_queue(bp->dev, queue_index), + packets, bytes); + /* Set end of TX queue */ desc = macb_tx_desc(queue, 0); macb_set_addr(bp, desc, 0); @@ -1230,6 +1237,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) unsigned int tail; unsigned int head; int packets = 0; + u32 bytes = 0; spin_lock(&queue->tx_ptr_lock); head = queue->tx_head; @@ -1271,6 +1279,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) bp->dev->stats.tx_bytes += skb->len; queue->stats.tx_bytes += skb->len; packets++; + bytes += skb->len; } /* Now we can safely release resources */ @@ -1285,6 +1294,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) } } + netdev_tx_completed_queue(netdev_get_tx_queue(bp->dev, queue_index), + packets, bytes); + queue->tx_tail = tail; if (__netif_subqueue_stopped(bp->dev, queue_index) && CIRC_CNT(queue->tx_head, queue->tx_tail, @@ -2386,6 +2398,8 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev) /* Make newly initialized descriptor visible to hardware */ wmb(); skb_tx_timestamp(skb); + netdev_tx_sent_queue(netdev_get_tx_queue(bp->dev, queue_index), + skb->len); spin_lock_irq(&bp->lock); macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART)); @@ -3019,6 +3033,7 @@ static int macb_close(struct net_device *dev) netif_tx_stop_all_queues(dev); for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { + netdev_tx_reset_queue(netdev_get_tx_queue(dev, q)); napi_disable(&queue->napi_rx); napi_disable(&queue->napi_tx); }
Implement byte queue limits to allow queuing disciplines to account for packets enqueued in the ring buffer but not yet transmitted. There are a separate set of transmit functions for AT91 that I haven't touched since I don't have hardware to test on. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/net/ethernet/cadence/macb_main.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)