Fri, Jun 07, 2024 at 08:47:43AM CEST, jasowang@xxxxxxxxxx wrote: >On Fri, Jun 7, 2024 at 2:39 PM Jiri Pirko <jiri@xxxxxxxxxxx> wrote: >> >> Fri, Jun 07, 2024 at 08:25:19AM CEST, jasowang@xxxxxxxxxx wrote: >> >On Thu, Jun 6, 2024 at 9:45 PM Jiri Pirko <jiri@xxxxxxxxxxx> wrote: >> >> >> >> Thu, Jun 06, 2024 at 09:56:50AM CEST, jasowang@xxxxxxxxxx wrote: >> >> >On Thu, Jun 6, 2024 at 2:05 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx> >> >> >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@xxxxxxxxxx> >> >> > Acked-by: Yevgeny Petrilin <yevgenyp@xxxxxxxxxxxx> >> >> > Cc: Or Gerlitz <ogerlitz@xxxxxxxxxxxx> >> >> > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx> >> >> > >> >> >> >> >> >> 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. > >True. Personally, I would like to just drop orphan mode. But I'm not >sure others are happy with this. How about to do it other way around. I will take a stab at sending patch removing it. If anyone is against and has solid data to prove orphan mode is needed, let them provide those. > >Thanks > >> >> >> >2) keep the module parameters >> >> and ignore them, correct? Perhaps a warning would be good. >> >> >> > >> >Thanks >> > >> >> >> >> >> >> >> >> > >> >> >Thanks >> >> > >> >> >> >> >> >> -- >> >> >> MST >> >> >> >> >> > >> >> >> > >> >
