Subject: Re: [patch net-next] virtio_net: add support for Byte Queue Limits

From: "Michael S. Tsirkin" <[email-masked]>
Date: Fri, 7 Jun 2024 02:39:53 -0400

On Fri, Jun 07, 2024 at 02:22:31PM +0800, Jason Wang wrote:
> On Thu, Jun 6, 2024 at 9:41 PM Jiri Pirko <[email-masked]> wrote:
> >
> > Thu, Jun 06, 2024 at 06:25:15AM CEST, [email-masked] wrote:
> > >On Thu, Jun 6, 2024 at 10:59 AM Jason Xing <[email-masked]> wrote:
> > >>
> > >> Hello Jason,
> > >>
> > >> On Thu, Jun 6, 2024 at 8:21 AM Jason Wang <[email-masked]> wrote:
> > >> >
> > >> > On Wed, Jun 5, 2024 at 7:51 PM Heng Qi <[email-masked]> wrote:
> > >> > >
> > >> > > On Wed, 5 Jun 2024 13:30:51 +0200, Jiri Pirko <[email-masked]> wrote:
> > >> > > > Mon, May 20, 2024 at 02:48:15PM CEST, [email-masked] wrote:
> > >> > > > >Fri, May 10, 2024 at 09:11:16AM CEST, [email-masked] wrote:
> > >> > > > >>On Thu,  9 May 2024 13:46:15 +0200, Jiri Pirko <[email-masked]> wrote:
> > >> > > > >>> From: Jiri Pirko <[email-masked]>
> > >> > > > >>>
> > >> > > > >>> Add support for Byte Queue Limits (BQL).
> > >> > > > >>
> > >> > > > >>Historically both Jason and Michael have attempted to support BQL
> > >> > > > >>for virtio-net, for example:
> > >> > > > >>
> > >> > > > >>https://lore.kernel.org/netdev/[email-masked]/
> > >> > > > >>
> > >> > > > >>These discussions focus primarily on:
> > >> > > > >>
> > >> > > > >>1. BQL is based on napi tx. Therefore, the transfer of statistical information
> > >> > > > >>needs to rely on the judgment of use_napi. When the napi mode is switched to
> > >> > > > >>orphan, some statistical information will be lost, resulting in temporary
> > >> > > > >>inaccuracy in BQL.
> > >> > > > >>
> > >> > > > >>2. If tx dim is supported, orphan mode may be removed and tx irq will be more
> > >> > > > >>reasonable. This provides good support for BQL.
> > >> > > > >
> > >> > > > >But when the device does not support dim, the orphan mode is still
> > >> > > > >needed, isn't it?
> > >> > > >
> > >> > > > Heng, is my assuption correct here? Thanks!
> > >> > > >
> > >> > >
> > >> > > Maybe, according to our cloud data, napi_tx=on works better than orphan mode in
> > >> > > most scenarios. Although orphan mode performs better in specific benckmark,
> > >> >
> > >> > For example pktgen (I meant even if the orphan mode can break pktgen,
> > >> > it can finish when there's a new packet that needs to be sent after
> > >> > pktgen is completed).
> > >> >
> > >> > > perf of napi_tx can be enhanced through tx dim. Then, there is no reason not to
> > >> > > support dim for devices that want the best performance.
> > >> >
> > >> > Ideally, if we can drop orphan mode, everything would be simplified.
> > >>
> > >> Please please don't do this. Orphan mode still has its merits. In some
> > >> cases which can hardly be reproduced in production, we still choose to
> > >> turn off the napi_tx mode because the delay of freeing a skb could
> > >> cause lower performance in the tx path,
> > >
> > >Well, it's probably just a side effect and it depends on how to define
> > >performance here.
> > >
> > >> which is, I know, surely
> > >> designed on purpose.
> > >
> > >I don't think so and no modern NIC uses that. It breaks a lot of things.
> > >
> > >>
> > >> If the codes of orphan mode don't have an impact when you enable
> > >> napi_tx mode, please keep it if you can.
> > >
> > >For example, it complicates BQL implementation.
> >
> > Well, bql could be disabled when napi is not used. It is just a matter
> > of one "if" in the xmit path.
> 
> Maybe, care to post a patch?
> 
> The trick part is, a skb is queued when BQL is enabled but sent when
> BQL is disabled as discussed here:
> 
> https://lore.kernel.org/netdev/[email-masked]/
> 
> Thanks

Yes of course. Or we can stick a dummy value in skb->destructor after we
orphan, maybe that's easier.

> > >Thanks
> > >> Thank you.