[PATCH net] net: ntb_netdev: Fix statistics races

[PATCH net] net: ntb_netdev: Fix statistics races

[Koichiro Den]

ntb_netdev updates shared net_device stats from per-QP RX and TX
callbacks. Once multiple queues are enabled, concurrent updates can be
lost.

Use core-managed per-CPU dstats for packet, byte and drop counters.
Keep infrequent error counters in net_device_stats with atomic
DEV_STATS_INC(). The core handles allocation and aggregation.

Transport queues can still complete after ndo_stop. Tear them down from
ndo_uninit before the core frees dstats.

Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
This is the follow-up mentioned here:
https://lore.kernel.org/r/20260819172539.1450821-1-den@valinux.co.jp/
The related TX and RX fixes have now landed in net.

 drivers/net/ntb_netdev.c | 44 ++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 9c171697e762..6a1e58d5d7d6 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -139,17 +139,16 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	netdev_dbg(ndev, "%s: %d byte payload received\n", __func__, len);
 
 	if (len < 0) {
-		ndev->stats.rx_errors++;
-		ndev->stats.rx_length_errors++;
+		DEV_STATS_INC(ndev, rx_errors);
+		DEV_STATS_INC(ndev, rx_length_errors);
 		goto enqueue_again;
 	}
 
-	ndev->stats.rx_packets++;
-	ndev->stats.rx_bytes += len;
+	dev_dstats_rx_add(ndev, len);
 
 	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
 	if (!new_skb) {
-		ndev->stats.rx_dropped++;
+		dev_dstats_rx_dropped(ndev);
 		goto enqueue_again;
 	}
 
@@ -166,8 +165,8 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, ndev->mtu + ETH_HLEN);
 	if (rc) {
 		dev_kfree_skb_any(skb);
-		ndev->stats.rx_errors++;
-		ndev->stats.rx_fifo_errors++;
+		DEV_STATS_INC(ndev, rx_errors);
+		DEV_STATS_INC(ndev, rx_fifo_errors);
 	}
 }
 
@@ -219,11 +218,13 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		return;
 
 	if (len > 0) {
-		ndev->stats.tx_packets++;
-		ndev->stats.tx_bytes += skb->len;
+		/* TX completion may run from the memcpy kthread. */
+		local_bh_disable();
+		dev_dstats_tx_add(ndev, skb->len);
+		local_bh_enable();
 	} else {
-		ndev->stats.tx_errors++;
-		ndev->stats.tx_aborted_errors++;
+		DEV_STATS_INC(ndev, tx_errors);
+		DEV_STATS_INC(ndev, tx_aborted_errors);
 	}
 
 	dev_kfree_skb_any(skb);
@@ -277,7 +278,7 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 
 drop:
 	dev_kfree_skb_any(skb);
-	ndev->stats.tx_dropped++;
+	dev_dstats_tx_dropped(ndev);
 	return NETDEV_TX_OK;
 }
 
@@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 	return rc;
 }
 
+static void ntb_netdev_uninit(struct net_device *ndev)
+{
+	struct ntb_netdev *dev = netdev_priv(ndev);
+	unsigned int q;
+
+	for (q = 0; q < dev->num_queues; q++) {
+		ntb_transport_free_queue(dev->queues[q].qp);
+		dev->queues[q].qp = NULL;
+	}
+}
+
 static const struct net_device_ops ntb_netdev_ops = {
+	.ndo_uninit = ntb_netdev_uninit,
 	.ndo_open = ntb_netdev_open,
 	.ndo_stop = ntb_netdev_close,
 	.ndo_start_xmit = ntb_netdev_start_xmit,
@@ -647,6 +660,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 	}
 
 	ndev->features = NETIF_F_HIGHDMA;
+	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
 
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 
@@ -696,8 +710,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 	return 0;
 
 err_free_qps:
-	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_free_queue(dev->queues[q].qp);
+	ntb_netdev_uninit(ndev);
 
 err_free_queues:
 	kfree(dev->queues);
@@ -711,11 +724,8 @@ static void ntb_netdev_remove(struct device *client_dev)
 {
 	struct net_device *ndev = dev_get_drvdata(client_dev);
 	struct ntb_netdev *dev = netdev_priv(ndev);
-	unsigned int q;
 
 	unregister_netdev(ndev);
-	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_free_queue(dev->queues[q].qp);
 
 	kfree(dev->queues);
 	free_netdev(ndev);

base-commit: 7cbfb180945ce529608e4d4e24a6d483699fab1e
-- 
2.51.0

Re: [PATCH net] net: ntb_netdev: Fix statistics races

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Pending RX and TX entries are leaked because their associated SKB buffers are not freed when the transport queue is destroyed.
--

--- Patch [1]: [PATCH net] net: ntb_netdev: Fix statistics races ---
commit fc558d0a991ec95505df001446270417159e2eaf
Author: Koichiro Den <den@valinux.co.jp>

net: ntb_netdev: Fix statistics races

ntb_netdev updates shared net_device stats from per-QP RX and TX callbacks.
This commit shifts the driver to use core-managed per-CPU dstats. It also
tears down transport queues from ndo_uninit before the core frees dstats.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 9c171697e7620..6a1e58d5d7d67 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
[ ... ]
> @@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
>  	return rc;
>  }
>  
> +static void ntb_netdev_uninit(struct net_device *ndev)
> +{
> +	struct ntb_netdev *dev = netdev_priv(ndev);
> +	unsigned int q;
> +
> +	for (q = 0; q < dev->num_queues; q++) {
> +		ntb_transport_free_queue(dev->queues[q].qp);

[Severity: High]
This is a pre-existing issue, but does calling ntb_transport_free_queue()
leak the SKBs attached to pending packets?

Looking at ntb_transport_free_queue() in drivers/ntb/ntb_transport.c,
it cleans up pending items from the queues like this:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
        dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
        kfree(entry);
    }
    ...
}

It frees the entry, but it doesn't seem to free or return the attached
client buffer (entry->cb_data). Since ntb_netdev uses cb_data to store
allocated SKBs, does this unconditionally leak any in-flight packets
when the queue is destroyed?

> +		dev->queues[q].qp = NULL;
> +	}
> +}
> +
>  static const struct net_device_ops ntb_netdev_ops = {
> +	.ndo_uninit = ntb_netdev_uninit,
>  	.ndo_open = ntb_netdev_open,
>  	.ndo_stop = ntb_netdev_close,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824025720.3520288-1-den@valinux.co.jp?part=1

Re: [PATCH net] net: ntb_netdev: Fix statistics races

[Simon Horman]

On Tue, Aug 25, 2026 at 02:57:39AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Pending RX and TX entries are leaked because their associated SKB buffers are not freed when the transport queue is destroyed.
> --
> 
> --- Patch [1]: [PATCH net] net: ntb_netdev: Fix statistics races ---
> commit fc558d0a991ec95505df001446270417159e2eaf
> Author: Koichiro Den <den@valinux.co.jp>
> 
> net: ntb_netdev: Fix statistics races
> 
> ntb_netdev updates shared net_device stats from per-QP RX and TX callbacks.
> This commit shifts the driver to use core-managed per-CPU dstats. It also
> tears down transport queues from ndo_uninit before the core frees dstats.
> 
> > diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> > index 9c171697e7620..6a1e58d5d7d67 100644
> > --- a/drivers/net/ntb_netdev.c
> > +++ b/drivers/net/ntb_netdev.c
> [ ... ]
> > @@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
> >  	return rc;
> >  }
> >  
> > +static void ntb_netdev_uninit(struct net_device *ndev)
> > +{
> > +	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	unsigned int q;
> > +
> > +	for (q = 0; q < dev->num_queues; q++) {
> > +		ntb_transport_free_queue(dev->queues[q].qp);
> 
> [Severity: High]
> This is a pre-existing issue, but does calling ntb_transport_free_queue()
> leak the SKBs attached to pending packets?

FWIIW, as a pre-existing issue I lean towards this being out-of-scope
for the patch under review. And, rather, a topic for possible follow-up.

> 
> Looking at ntb_transport_free_queue() in drivers/ntb/ntb_transport.c,
> it cleans up pending items from the queues like this:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>     ...
>     while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
>         dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
>         kfree(entry);
>     }
>     ...
> }
> 
> It frees the entry, but it doesn't seem to free or return the attached
> client buffer (entry->cb_data). Since ntb_netdev uses cb_data to store
> allocated SKBs, does this unconditionally leak any in-flight packets
> when the queue is destroyed?
> 
> > +		dev->queues[q].qp = NULL;
> > +	}
> > +}
> > +
> >  static const struct net_device_ops ntb_netdev_ops = {
> > +	.ndo_uninit = ntb_netdev_uninit,
> >  	.ndo_open = ntb_netdev_open,
> >  	.ndo_stop = ntb_netdev_close,
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260824025720.3520288-1-den@valinux.co.jp?part=1

Re: [PATCH net] net: ntb_netdev: Fix statistics races

[Simon Horman]

On Mon, Aug 24, 2026 at 11:57:20AM +0900, Koichiro Den wrote:
> ntb_netdev updates shared net_device stats from per-QP RX and TX
> callbacks. Once multiple queues are enabled, concurrent updates can be
> lost.
> 
> Use core-managed per-CPU dstats for packet, byte and drop counters.
> Keep infrequent error counters in net_device_stats with atomic
> DEV_STATS_INC(). The core handles allocation and aggregation.
> 
> Transport queues can still complete after ndo_stop. Tear them down from
> ndo_uninit before the core frees dstats.
> 
> Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
> This is the follow-up mentioned here:
> https://lore.kernel.org/r/20260819172539.1450821-1-den@valinux.co.jp/
> The related TX and RX fixes have now landed in net.
> 
>  drivers/net/ntb_netdev.c | 44 ++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 9c171697e762..6a1e58d5d7d6 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c

...

> @@ -219,11 +218,13 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  		return;
>  
>  	if (len > 0) {
> -		ndev->stats.tx_packets++;
> -		ndev->stats.tx_bytes += skb->len;
> +		/* TX completion may run from the memcpy kthread. */
> +		local_bh_disable();
> +		dev_dstats_tx_add(ndev, skb->len);
> +		local_bh_enable();


Hi Den-san,

There is an AI-generated review of this patch available at [1].
This is separate to the AI-generated review forwarded elswhere in this
thread by sashiko-bot.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824025720.3520288-1-den%40valinux.co.jp

It raises a concern regarding using local_bh_disable/local_bh_enable.
And I wonder if this might be addressed by unrolling dev_dstats_tx_add,
something like this:

		u64_stats_update_begin_irqsave(&dstats->syncp, syncp_flags);
		u64_stats_inc(&dstats->tx_packets);
		u64_stats_add(&dstats->tx_bytes, skb->len);
		u64_stats_update_end_irqrestore(&dstats->syncp, syncp_flags);

AI-generated review text:

  Can ntb_netdev_tx_handler() run with interrupts disabled here?

  With the ntb_transport defaults (use_dma=false, tx_memcpy_offload=false)
  the TX completion is not deferred to a kthread at all, it runs inline in
  the ndo_start_xmit caller's context:

  ntb_netdev_start_xmit()
    ntb_transport_tx_enqueue()
      ntb_process_tx()
        ntb_async_tx()            /* chan == NULL, goto err */
          ntb_memcpy_tx()
            ntb_memcpy_tx_on_stack()
              ntb_tx_copy_callback()
                qp->tx_handler()  == ntb_netdev_tx_handler()

  netpoll calls ndo_start_xmit with interrupts off:

  net/core/netpoll.c:netpoll_send_skb() {
	...
	local_irq_save(flags);
	ret = __netpoll_send_skb(np, skb);
	local_irq_restore(flags);
	...
  }

  and ntb_netdev does not set IFF_DISABLE_NETPOLL, so netconsole can bind
  to the interface. In that context local_bh_enable() hits:

  kernel/softirq.c:__local_bh_enable_ip() {
	WARN_ON_ONCE(in_hardirq());
	lockdep_assert_irqs_enabled();
	...
	if (unlikely(!in_interrupt() && local_softirq_pending()))
		do_softirq();
	...
  }

  so besides the splat, softirqs would be run from inside a netpoll/printk
  section that expects interrupts to stay disabled.

  The pre-existing dev_kfree_skb_any() a couple of lines below already
  assumes this handler can be entered from hardirq or with IRQs disabled,
  which is the same assumption the plain ndev->stats.tx_packets++ was safe
  under.

  There is also a second entry path: ntb_async_tx_submit() installs
  ntb_tx_copy_callback() as txd->callback_result, and whether a dmaengine
  provider calls that from hardirq is up to the provider. If it does, a
  BH-disable is not enough to protect the pcpu_dstats syncp that the RX
  side writes from the rxc_db tasklet on the same CPU, so on 32-bit the
  seqcount can be torn.

  Would an IRQ-safe update (local_irq_save()/local_irq_restore() around
  the update, or u64_stats_update_begin_irqsave() on
  this_cpu_ptr(ndev->dstats)) be correct in all of these contexts?

>  	} else {
> -		ndev->stats.tx_errors++;
> -		ndev->stats.tx_aborted_errors++;
> +		DEV_STATS_INC(ndev, tx_errors);
> +		DEV_STATS_INC(ndev, tx_aborted_errors);
>  	}
>  
>  	dev_kfree_skb_any(skb);
> @@ -277,7 +278,7 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  
>  drop:
>  	dev_kfree_skb_any(skb);
> -	ndev->stats.tx_dropped++;
> +	dev_dstats_tx_dropped(ndev);
>  	return NETDEV_TX_OK;
>  }
>  
> @@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
>  	return rc;
>  }
>  
> +static void ntb_netdev_uninit(struct net_device *ndev)
> +{
> +	struct ntb_netdev *dev = netdev_priv(ndev);
> +	unsigned int q;
> +
> +	for (q = 0; q < dev->num_queues; q++) {
> +		ntb_transport_free_queue(dev->queues[q].qp);
> +		dev->queues[q].qp = NULL;
> +	}
> +}
> +

[1] also raises a concern here relating to holding RTNL for too long.

I wonder if this can be addressed by quiescing the queues in
ntb_netdev_uninit() while leaving the calls ntb_transport_free_queue()
until after unregister_netdev() has been called in ntb_netdev_remove().

AI-generated review text:

  How long can RTNL be held by this new ndo_uninit path?

  ndo_uninit is called from unregister_netdevice_many_notify() with RTNL
  held (unregister_netdev() -> rtnl_net_dev_lock()):

  net/core/dev.c:unregister_netdevice_many_notify() {
	...
		if (dev->netdev_ops->ndo_uninit)
			dev->netdev_ops->ndo_uninit(dev);
	...
  }

  ntb_transport_free_queue() then drains the DMA channels for each queue
  pair, once for TX and once for RX:

  drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
	...
		dma_sync_wait(chan, qp->last_cookie);
		dmaengine_terminate_all(chan);
	...
  }

  and dma_sync_wait() is a busy-poll with a 5 second timeout and no early
  abort:

  drivers/dma/dmaengine.c:dma_sync_wait() {
	unsigned long dma_sync_wait_timeout = jiffies + msecs_to_jiffies(5000);
	dma_async_issue_pending(chan);
	do {
		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
		if (time_after_eq(jiffies, dma_sync_wait_timeout)) {
			dev_err(chan->device->dev, "%s: timeout!\n", __func__);
			return DMA_ERROR;
		}
		if (status != DMA_IN_PROGRESS)
			break;
		cpu_relax();
	} while (1);
	...
  }

  With use_dma=1 and a wedged channel or a downed peer, that is up to
  roughly 10 seconds of spinning per queue pair, multiplied by
  dev->num_queues, plus kthread_stop(), tasklet_kill() and
  cancel_delayed_work_sync(), all under RTNL. Before this patch the same
  loop ran in ntb_netdev_remove() after unregister_netdev() returned, with
  no locks held.

  Since the stats fix only needs the transport callbacks to stop before
  the core frees dstats, could the channel drain and release stay in
  ntb_netdev_remove()?

>  static const struct net_device_ops ntb_netdev_ops = {
> +	.ndo_uninit = ntb_netdev_uninit,
>  	.ndo_open = ntb_netdev_open,
>  	.ndo_stop = ntb_netdev_close,
>  	.ndo_start_xmit = ntb_netdev_start_xmit,
> @@ -647,6 +660,7 @@ static int ntb_netdev_probe(struct device *client_dev)
>  	}
>  
>  	ndev->features = NETIF_F_HIGHDMA;
> +	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
>  
>  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>  
> @@ -696,8 +710,7 @@ static int ntb_netdev_probe(struct device *client_dev)
>  	return 0;
>  
>  err_free_qps:
> -	for (q = 0; q < dev->num_queues; q++)
> -		ntb_transport_free_queue(dev->queues[q].qp);
> +	ntb_netdev_uninit(ndev);
>  
>  err_free_queues:
>  	kfree(dev->queues);
> @@ -711,11 +724,8 @@ static void ntb_netdev_remove(struct device *client_dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(client_dev);
>  	struct ntb_netdev *dev = netdev_priv(ndev);
> -	unsigned int q;
>  
>  	unregister_netdev(ndev);
> -	for (q = 0; q < dev->num_queues; q++)
> -		ntb_transport_free_queue(dev->queues[q].qp);
>  
>  	kfree(dev->queues);
>  	free_netdev(ndev);
> 
> base-commit: 7cbfb180945ce529608e4d4e24a6d483699fab1e
> -- 
> 2.51.0
> 

Re: [PATCH net] net: ntb_netdev: Fix statistics races

[Koichiro Den]

On Thu, Aug 27, 2026 at 12:04:45PM +0100, Simon Horman wrote:
> On Mon, Aug 24, 2026 at 11:57:20AM +0900, Koichiro Den wrote:
> > ntb_netdev updates shared net_device stats from per-QP RX and TX
> > callbacks. Once multiple queues are enabled, concurrent updates can be
> > lost.
> > 
> > Use core-managed per-CPU dstats for packet, byte and drop counters.
> > Keep infrequent error counters in net_device_stats with atomic
> > DEV_STATS_INC(). The core handles allocation and aggregation.
> > 
> > Transport queues can still complete after ndo_stop. Tear them down from
> > ndo_uninit before the core frees dstats.
> > 
> > Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> > This is the follow-up mentioned here:
> > https://lore.kernel.org/r/20260819172539.1450821-1-den@valinux.co.jp/
> > The related TX and RX fixes have now landed in net.
> > 
> >  drivers/net/ntb_netdev.c | 44 ++++++++++++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> > index 9c171697e762..6a1e58d5d7d6 100644
> > --- a/drivers/net/ntb_netdev.c
> > +++ b/drivers/net/ntb_netdev.c
> 
> ...
> 
> > @@ -219,11 +218,13 @@ static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp, void *qp_data,
> >  		return;
> >  
> >  	if (len > 0) {
> > -		ndev->stats.tx_packets++;
> > -		ndev->stats.tx_bytes += skb->len;
> > +		/* TX completion may run from the memcpy kthread. */
> > +		local_bh_disable();
> > +		dev_dstats_tx_add(ndev, skb->len);
> > +		local_bh_enable();
> 
> 
> Hi Den-san,
> 
> There is an AI-generated review of this patch available at [1].
> This is separate to the AI-generated review forwarded elswhere in this
> thread by sashiko-bot.
> 
> [1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824025720.3520288-1-den%40valinux.co.jp

Hi Simon,

Thanks. I missed both the netdev-ai report (again) and the netpoll context.
I'll respin this.

> 
> It raises a concern regarding using local_bh_disable/local_bh_enable.
> And I wonder if this might be addressed by unrolling dev_dstats_tx_add,
> something like this:
> 
> 		u64_stats_update_begin_irqsave(&dstats->syncp, syncp_flags);
> 		u64_stats_inc(&dstats->tx_packets);
> 		u64_stats_add(&dstats->tx_bytes, skb->len);
> 		u64_stats_update_end_irqrestore(&dstats->syncp, syncp_flags);

Looking around, I'm thinking of following the same pattern as
mctp_usblib_tx_stats_update(), using tstats:

  tstats = get_cpu_ptr(ndev->tstats);
  flags = u64_stats_update_begin_irqsave(&tstats->syncp);
  ...
  u64_stats_update_end_irqrestore(&tstats->syncp, flags);
  put_cpu_ptr(ndev->tstats);

Leaving pcpu_stat_type as NONE makes it driver-owned, so they can remain alive
until ntb_transport_free_queue() has finished. This should let me restore the
original teardown order, with QP teardown after unregister_netdev(), without
moving the DMA waits under RTNL.

Best regards,
Koichiro

> 
> AI-generated review text:
> 
>   Can ntb_netdev_tx_handler() run with interrupts disabled here?
> 
>   With the ntb_transport defaults (use_dma=false, tx_memcpy_offload=false)
>   the TX completion is not deferred to a kthread at all, it runs inline in
>   the ndo_start_xmit caller's context:
> 
>   ntb_netdev_start_xmit()
>     ntb_transport_tx_enqueue()
>       ntb_process_tx()
>         ntb_async_tx()            /* chan == NULL, goto err */
>           ntb_memcpy_tx()
>             ntb_memcpy_tx_on_stack()
>               ntb_tx_copy_callback()
>                 qp->tx_handler()  == ntb_netdev_tx_handler()
> 
>   netpoll calls ndo_start_xmit with interrupts off:
> 
>   net/core/netpoll.c:netpoll_send_skb() {
> 	...
> 	local_irq_save(flags);
> 	ret = __netpoll_send_skb(np, skb);
> 	local_irq_restore(flags);
> 	...
>   }
> 
>   and ntb_netdev does not set IFF_DISABLE_NETPOLL, so netconsole can bind
>   to the interface. In that context local_bh_enable() hits:
> 
>   kernel/softirq.c:__local_bh_enable_ip() {
> 	WARN_ON_ONCE(in_hardirq());
> 	lockdep_assert_irqs_enabled();
> 	...
> 	if (unlikely(!in_interrupt() && local_softirq_pending()))
> 		do_softirq();
> 	...
>   }
> 
>   so besides the splat, softirqs would be run from inside a netpoll/printk
>   section that expects interrupts to stay disabled.
> 
>   The pre-existing dev_kfree_skb_any() a couple of lines below already
>   assumes this handler can be entered from hardirq or with IRQs disabled,
>   which is the same assumption the plain ndev->stats.tx_packets++ was safe
>   under.
> 
>   There is also a second entry path: ntb_async_tx_submit() installs
>   ntb_tx_copy_callback() as txd->callback_result, and whether a dmaengine
>   provider calls that from hardirq is up to the provider. If it does, a
>   BH-disable is not enough to protect the pcpu_dstats syncp that the RX
>   side writes from the rxc_db tasklet on the same CPU, so on 32-bit the
>   seqcount can be torn.
> 
>   Would an IRQ-safe update (local_irq_save()/local_irq_restore() around
>   the update, or u64_stats_update_begin_irqsave() on
>   this_cpu_ptr(ndev->dstats)) be correct in all of these contexts?
> 
> >  	} else {
> > -		ndev->stats.tx_errors++;
> > -		ndev->stats.tx_aborted_errors++;
> > +		DEV_STATS_INC(ndev, tx_errors);
> > +		DEV_STATS_INC(ndev, tx_aborted_errors);
> >  	}
> >  
> >  	dev_kfree_skb_any(skb);
> > @@ -277,7 +278,7 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
> >  
> >  drop:
> >  	dev_kfree_skb_any(skb);
> > -	ndev->stats.tx_dropped++;
> > +	dev_dstats_tx_dropped(ndev);
> >  	return NETDEV_TX_OK;
> >  }
> >  
> > @@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
> >  	return rc;
> >  }
> >  
> > +static void ntb_netdev_uninit(struct net_device *ndev)
> > +{
> > +	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	unsigned int q;
> > +
> > +	for (q = 0; q < dev->num_queues; q++) {
> > +		ntb_transport_free_queue(dev->queues[q].qp);
> > +		dev->queues[q].qp = NULL;
> > +	}
> > +}
> > +
> 
> [1] also raises a concern here relating to holding RTNL for too long.
> 
> I wonder if this can be addressed by quiescing the queues in
> ntb_netdev_uninit() while leaving the calls ntb_transport_free_queue()
> until after unregister_netdev() has been called in ntb_netdev_remove().
> 
> AI-generated review text:
> 
>   How long can RTNL be held by this new ndo_uninit path?
> 
>   ndo_uninit is called from unregister_netdevice_many_notify() with RTNL
>   held (unregister_netdev() -> rtnl_net_dev_lock()):
> 
>   net/core/dev.c:unregister_netdevice_many_notify() {
> 	...
> 		if (dev->netdev_ops->ndo_uninit)
> 			dev->netdev_ops->ndo_uninit(dev);
> 	...
>   }
> 
>   ntb_transport_free_queue() then drains the DMA channels for each queue
>   pair, once for TX and once for RX:
> 
>   drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
> 	...
> 		dma_sync_wait(chan, qp->last_cookie);
> 		dmaengine_terminate_all(chan);
> 	...
>   }
> 
>   and dma_sync_wait() is a busy-poll with a 5 second timeout and no early
>   abort:
> 
>   drivers/dma/dmaengine.c:dma_sync_wait() {
> 	unsigned long dma_sync_wait_timeout = jiffies + msecs_to_jiffies(5000);
> 	dma_async_issue_pending(chan);
> 	do {
> 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> 		if (time_after_eq(jiffies, dma_sync_wait_timeout)) {
> 			dev_err(chan->device->dev, "%s: timeout!\n", __func__);
> 			return DMA_ERROR;
> 		}
> 		if (status != DMA_IN_PROGRESS)
> 			break;
> 		cpu_relax();
> 	} while (1);
> 	...
>   }
> 
>   With use_dma=1 and a wedged channel or a downed peer, that is up to
>   roughly 10 seconds of spinning per queue pair, multiplied by
>   dev->num_queues, plus kthread_stop(), tasklet_kill() and
>   cancel_delayed_work_sync(), all under RTNL. Before this patch the same
>   loop ran in ntb_netdev_remove() after unregister_netdev() returned, with
>   no locks held.
> 
>   Since the stats fix only needs the transport callbacks to stop before
>   the core frees dstats, could the channel drain and release stay in
>   ntb_netdev_remove()?
> 
> >  static const struct net_device_ops ntb_netdev_ops = {
> > +	.ndo_uninit = ntb_netdev_uninit,
> >  	.ndo_open = ntb_netdev_open,
> >  	.ndo_stop = ntb_netdev_close,
> >  	.ndo_start_xmit = ntb_netdev_start_xmit,
> > @@ -647,6 +660,7 @@ static int ntb_netdev_probe(struct device *client_dev)
> >  	}
> >  
> >  	ndev->features = NETIF_F_HIGHDMA;
> > +	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> >  
> >  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> >  
> > @@ -696,8 +710,7 @@ static int ntb_netdev_probe(struct device *client_dev)
> >  	return 0;
> >  
> >  err_free_qps:
> > -	for (q = 0; q < dev->num_queues; q++)
> > -		ntb_transport_free_queue(dev->queues[q].qp);
> > +	ntb_netdev_uninit(ndev);
> >  
> >  err_free_queues:
> >  	kfree(dev->queues);
> > @@ -711,11 +724,8 @@ static void ntb_netdev_remove(struct device *client_dev)
> >  {
> >  	struct net_device *ndev = dev_get_drvdata(client_dev);
> >  	struct ntb_netdev *dev = netdev_priv(ndev);
> > -	unsigned int q;
> >  
> >  	unregister_netdev(ndev);
> > -	for (q = 0; q < dev->num_queues; q++)
> > -		ntb_transport_free_queue(dev->queues[q].qp);
> >  
> >  	kfree(dev->queues);
> >  	free_netdev(ndev);
> > 
> > base-commit: 7cbfb180945ce529608e4d4e24a6d483699fab1e
> > -- 
> > 2.51.0
> >