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

Jiri Pirko
2024-06-07T06:39:25Z

Fri, Jun 07, 2024 at 08:25:19AM CEST, jasowang@redhat.com wrote:
>On Thu, Jun 6, 2024 at 9:45 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Jun 06, 2024 at 09:56:50AM CEST, jasowang@redhat.com wrote:
>> >On Thu, Jun 6, 2024 at 2:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >>
>> >> On Thu, Jun 06, 2024 at 12:25:15PM +0800, Jason Wang wrote:
>> >> > > 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.
>> >> >
>> >> > Thanks
>> >>
>> >> I very much doubt sending interrupts to a VM can
>> >> *on all benchmarks* compete with not sending interrupts.
>> >
>> >It should not differ too much from the physical NIC. We can have one
>> >more round of benchmarks to see the difference.
>> >
>> >But if NAPI mode needs to win all of the benchmarks in order to get
>> >rid of orphan, that would be very difficult. Considering various bugs
>> >will be fixed by dropping skb_orphan(), it would be sufficient if most
>> >of the benchmark doesn't show obvious differences.
>> >
>> >Looking at git history, there're commits that removes skb_orphan(), for example:
>> >
>> >commit 8112ec3b8722680251aecdcc23dfd81aa7af6340
>> >Author: Eric Dumazet <edumazet@google.com>
>> >Date: Fri Sep 28 07:53:26 2012 +0000
>> >
>> > mlx4: dont orphan skbs in mlx4_en_xmit()
>> >
>> > After commit e22979d96a55d (mlx4_en: Moving to Interrupts for TX
>> > completions) we no longer need to orphan skbs in mlx4_en_xmit()
>> > since skb wont stay a long time in TX ring before their release.
>> >
>> > Orphaning skbs in ndo_start_xmit() should be avoided as much as
>> > possible, since it breaks TCP Small Queue or other flow control
>> > mechanisms (per socket limits)
>> >
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>> > Acked-by: Yevgeny Petrilin <yevgenyp@mellanox.com>
>> > Cc: Or Gerlitz <ogerlitz@mellanox.com>
>> > Signed-off-by: David S. Miller <davem@davemloft.net>
>> >
>> >>
>> >> So yea, it's great if napi and hardware are advanced enough
>> >> that the default can be changed, since this way virtio
>> >> is closer to a regular nic and more or standard
>> >> infrastructure can be used.
>> >>
>> >> But dropping it will go against *no breaking userspace* rule.
>> >> Complicated? Tough.
>> >
>> >I don't know what kind of userspace is broken by this. Or why it is
>> >not broken since the day we enable NAPI mode by default.
>>
>> There is a module option that explicitly allows user to set
>> napi_tx=false
>> or
>> napi_weight=0
>>
>> So if you remove this option or ignore it, both breaks the user
>> expectation.
>
>We can keep them, but I wonder what's the expectation of the user
>here? The only thing so far I can imagine is the performance
>difference.

True.

>
>> I personally would vote for this breakage. To carry ancient
>> things like this one forever does not make sense to me.
>
>Exactly.
>
>> While at it,
>> let's remove all virtio net module params. Thoughts?
>
>I tend to
>
>1) drop the orphan mode, but we can have some benchmarks first

Any idea which? That would be really tricky to find the ones where
orphan mode makes difference I assume.

>2) keep the module parameters

and ignore them, correct? Perhaps a warning would be good.

>
>Thanks
>
>>
>>
>>
>> >
>> >Thanks
>> >
>> >>
>> >> --
>> >> MST
>> >>
>> >
>>
>