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

[Jason Wang <jasowang@redhat.com>]

On Mon, Dec 25, 2023 at 4:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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?

No, I just describe what I've seen from the list. No patches were
posted for performance optimization in recent years.

> I think it's just micro optimized to a level where
> progress is difficult.
>
>

Maybe.

Let me clarify what I meant.

If people can help to optimize the NAPI to be as fast as orphans or
something like 80%-90% of orphans. It should be sufficient to remove
the orphan path completely. This allows a lot of good features to be
built easily on top.

To me, it seems fine to remove it even now as it breaks socket
accounting which is actually a bug and NAPI has been enabled for years
but if you don't wish to do that, that's fine.  We can wait for the
coalescing and dim to be used for a while and revisit this.

Thanks