Re: [PATCH net-next] virtio-net: switch napi_tx without downing nic

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 25, 2023 at 12:12:48PM +0800, Jason Wang wrote:
> On Fri, Dec 22, 2023 at 4:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Dec 22, 2023 at 10:35:07AM +0800, Jason Wang wrote:
> > > On Thu, Dec 21, 2023 at 11:06 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Heng Qi wrote:
> > > > >
> > > > >
> > > > > 在 2023/12/20 下午10:45, Willem de Bruijn 写道:
> > > > > > Heng Qi wrote:
> > > > > >> virtio-net has two ways to switch napi_tx: one is through the
> > > > > >> module parameter, and the other is through coalescing parameter
> > > > > >> settings (provided that the nic status is down).
> > > > > >>
> > > > > >> Sometimes we face performance regression caused by napi_tx,
> > > > > >> then we need to switch napi_tx when debugging. However, the
> > > > > >> existing methods are a bit troublesome, such as needing to
> > > > > >> reload the driver or turn off the network card.
> > >
> > > Why is this troublesome? We don't need to turn off the card, it's just
> > > a toggling of the interface.
> > >
> > > This ends up with pretty simple code.
> > >
> > > > So try to make
> > > > > >> this update.
> > > > > >>
> > > > > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > The commit does not explain why it is safe to do so.
> > > > >
> > > > > virtnet_napi_tx_disable ensures that already scheduled tx napi ends and
> > > > > no new tx napi will be scheduled.
> > > > >
> > > > > Afterwards, if the __netif_tx_lock_bh lock is held, the stack cannot
> > > > > send the packet.
> > > > >
> > > > > Then we can safely toggle the weight to indicate where to clear the buffers.
> > > > >
> > > > > >
> > > > > > The tx-napi weights are not really weights: it is a boolean whether
> > > > > > napi is used for transmit cleaning, or whether packets are cleaned
> > > > > > in ndo_start_xmit.
> > > > >
> > > > > Right.
> > > > >
> > > > > >
> > > > > > There certainly are some subtle issues with regard to pausing/waking
> > > > > > queues when switching between modes.
> > > > >
> > > > > What are "subtle issues" and if there are any, we find them.
> > > >
> > > > A single runtime test is not sufficient to exercise all edge cases.
> > > >
> > > > Please don't leave it to reviewers to establish the correctness of a
> > > > patch.
> > >
> > > +1
> > >
> > > And instead of trying to do this, it would be much better to optimize
> > > the NAPI performance. Then we can drop the orphan mode.
> >
> > "To address your problem, optimize our code to the level which we
> > couldn't achieve in more than 10 years".
> 
> Last time QE didn't report any issue for TCP. For others, the code
> might just need some optimization if it really matters, it's just
> because nobody has worked on this part in the past years.

You think nobody worked on performance of virtio net because nobody
could bother? I think it's just micro optimized to a level where
progress is difficult.

> The ethtool trick is just for debugging purposes, I can hardly believe
> it is used by any management layer software.

It's UAPI - someone somewhere uses it, management layer or not.

-- 
MST