Date: Mon, 25 Dec 2023 12:14:45 +0800
From: Jason Wang <jasowang@...hat.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, Heng Qi <hengqi@...ux.alibaba.com>, 
	netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org, 
	"Michael S. Tsirkin" <mst@...hat.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH net-next] virtio-net: switch napi_tx without downing nic

On Mon, Dec 25, 2023 at 10:25 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Hello Jason,
> On Fri, Dec 22, 2023 at 10:36 AM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Thu, Dec 21, 2023 at 11:06 PM Willem de Bruijn
> > <willemdebruijn.kernel@...il.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@...ux.alibaba.com>
> > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@...ux.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.
>
> Do you mean when to call skb_orphan()? If yes, I just want to provide
> more information that we also have some performance issues where the
> flow control takes a bad effect, especially under some small
> throughput in our production environment.

I think you need to describe it in detail.

> What strikes me as odd is if I restart the network, then the issue
> will go with the wind. I cannot reproduce it in my testing machine.
> One more thing: if I force skb_orphan() the current skb in every
> start_xmit(), it could also solve the issue but not in a proper way.
> After all, it drops the flow control... :S

Yes, that's the known issue.

Thanks

>
> Thanks,
> Jason
> >
> > >
> > > The napi_tx and non-napi code paths differ in how they handle at least
> > > the following structures:
> > >
> > > 1. skb: non-napi orphans these in ndo_start_xmit. Without napi this is
> > > needed as delay until the next ndo_start_xmit and thus completion is
> > > unbounded.
> > >
> > > When switching to napi mode, orphaned skbs may now be cleaned by the
> > > napi handler. This is indeed safe.
> > >
> > > When switching from napi to non-napi, the unbound latency resurfaces.
> > > It is a small edge case, and I think a potentially acceptable risk, if
> > > the user of this knob is aware of the risk.
> > >
> > > 2. virtqueue callback ("interrupt" masking). The non-napi path enables
> > > the interrupt (disables the mask) when available descriptors falls
> > > beneath a low watermark, and reenables when it recovers above a high
> > > watermark. Napi disables when napi is scheduled, and reenables on
> > > napi complete.
> > >
> > > 3. dev_queue->state (QUEUE_STATE_DRV_XOFF). if the ring falls below
> > > a low watermark, the driver stops the stack for queuing more packets.
> > > In napi mode, it schedules napi to clean packets. See the calls to
> > > netif_xmit_stopped, netif_stop_subqueue, netif_start_subqueue and
> > > netif_tx_wake_queue.
> > >
> > > Some if this can be assumed safe by looking at existing analogous
> > > code, such as the queue stop/start in virtnet_tx_resize.
> > >
> > > But that all virtqueue callback and dev_queue->state transitions are
> > > correct when switching between modes at runtime is not trivial to
> > > establish, deserves some thought and explanation in the commit
> > > message.
> >
> > Thanks
> >
> >
>

Powered by blists - more mailing lists