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] [day] [month] [year] [list]

Message-ID: <20251119025546-mutt-send-email-mst@kernel.org>
Date: Wed, 19 Nov 2025 02:58:39 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Liming Wu <liming.wu@...uarmicro.com>
Cc: Jason Wang <jasowang@...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

On Wed, Nov 19, 2025 at 07:54:07AM +0000, Liming Wu wrote:
> > 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.


Please do not send fluff comments like this to the list.

And with em-dashes too, for added flair.

If you can not bother writing email yourself why should
anyone bother reading it?




> > 
> > >                                 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
>