linux-kernel - RE: [PATCH] virtio_net: enhance wake/stop tx queue statistics accounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <PSAPR06MB39429783A41F42FDD82477A2E1D7A@PSAPR06MB3942.apcprd06.prod.outlook.com>
Date: Wed, 19 Nov 2025 07:54:07 +0000
From: Liming Wu <liming.wu@...uarmicro.com>
To: Jason Wang <jasowang@...hat.com>
CC: "Michael S . Tsirkin" <mst@...hat.com>, Xuan Zhuo
	<xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
	<eperezma@...hat.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"virtualization@...ts.linux-foundation.org"
	<virtualization@...ts.linux-foundation.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Angus Chen <angus.chen@...uarmicro.com>
Subject: RE: [PATCH] virtio_net: enhance wake/stop tx queue statistics
 accounting

> queue wake/stop events introduced by a previous patch.
> 
> It would be better to add commit id here.
OK, thx.

> 
eck. */
> >                         free_old_xmit(sq, txq, false);
> >                         if (sq->vq->num_free >= MAX_SKB_FRAGS + 2) {
> > -                               netif_start_subqueue(dev, qnum);
> > -
> u64_stats_update_begin(&sq->stats.syncp);
> > -                               u64_stats_inc(&sq->stats.wake);
> > -
> u64_stats_update_end(&sq->stats.syncp);
> > +                               virtnet_tx_wake_queue(vi, sq);
> 
> This is suspicious, netif_tx_wake_queue() will schedule qdisc, or is this intended?
Thanks for pointing this out.
You're right — using netif_tx_wake_queue() here would indeed trigger qdisc scheduling, which is not intended in this specific path.
My change tried to unify the wake/stop accounting paths, but replacing netif_start_subqueue() was not the right choice semantically.

I will restore netif_start_subqueue() at this site and keep only the statistic increment, so the behavior stays consistent with the original code while still improving the per-queue metrics.

> 
> >                                 virtqueue_disable_cb(sq->vq);
> >                         }
> >                 }
> > @@ -3068,13 +3080,8 @@ static void virtnet_poll_cleantx(struct
> receive_queue *rq, int budget)
> >                         free_old_xmit(sq, txq, !!budget);
> >                 } while
> > (unlikely(!virtqueue_enable_cb_delayed(sq->vq)));
> >
> > -               if (sq->vq->num_free >= MAX_SKB_FRAGS + 2 &&
> > -                   netif_tx_queue_stopped(txq)) {
> > -                       u64_stats_update_begin(&sq->stats.syncp);
> > -                       u64_stats_inc(&sq->stats.wake);
> > -                       u64_stats_update_end(&sq->stats.syncp);
> > -                       netif_tx_wake_queue(txq);
> > -               }
> > +               if (sq->vq->num_free >= MAX_SKB_FRAGS + 2)
> > +                       virtnet_tx_wake_queue(vi, sq);
> >
> >                 __netif_tx_unlock(txq);
> >         }
> > @@ -3264,13 +3271,8 @@ static int virtnet_poll_tx(struct napi_struct *napi,
> int budget)
> >         else
> >                 free_old_xmit(sq, txq, !!budget);
> >
> > -       if (sq->vq->num_free >= MAX_SKB_FRAGS + 2 &&
> > -           netif_tx_queue_stopped(txq)) {
> > -               u64_stats_update_begin(&sq->stats.syncp);
> > -               u64_stats_inc(&sq->stats.wake);
> > -               u64_stats_update_end(&sq->stats.syncp);
> > -               netif_tx_wake_queue(txq);
> > -       }
> > +       if (sq->vq->num_free >= MAX_SKB_FRAGS + 2)
> > +               virtnet_tx_wake_queue(vi, sq);
> >
> >         if (xsk_done >= budget) {
> >                 __netif_tx_unlock(txq); @@ -3521,6 +3523,9 @@ static
> > void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
> >
> >         /* Prevent the upper layer from trying to send packets. */
> >         netif_stop_subqueue(vi->dev, qindex);
> > +       u64_stats_update_begin(&sq->stats.syncp);
> > +       u64_stats_inc(&sq->stats.stop);
> > +       u64_stats_update_end(&sq->stats.syncp);
> >
> >         __netif_tx_unlock_bh(txq);
> >  }
> > @@ -3537,7 +3542,7 @@ static void virtnet_tx_resume(struct
> > virtnet_info *vi, struct send_queue *sq)
> >
> >         __netif_tx_lock_bh(txq);
> >         sq->reset = false;
> > -       netif_tx_wake_queue(txq);
> > +       virtnet_tx_wake_queue(vi, sq);
> >         __netif_tx_unlock_bh(txq);
> >
> >         if (running)
> > --
> > 2.34.1
> >
> 
> Thanks