Re: [PATCH net-next] virtio_net: Add TX stop and wake counters

On Mon, Feb 05, 2024 at 09:45:38AM +0800, Jason Wang wrote:

On Sun, Feb 4, 2024 at 8:39 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Sun, Feb 04, 2024 at 09:20:18AM +0800, Jason Wang wrote:

quoted

On Sat, Feb 3, 2024 at 12:01 AM Jakub Kicinski [off-list ref] wrote:

quoted

On Fri, 2 Feb 2024 14:52:59 +0800 Jason Xing wrote:

quoted

quoted

Can you say more? I'm curious what's your use case.

I'm not working at Nvidia, so my point of view may differ from theirs.
From what I can tell is that those two counters help me narrow down
the range if I have to diagnose/debug some issues.

right, i'm asking to collect useful debugging tricks, nothing against
the patch itself :)

quoted

1) I sometimes notice that if some irq is held too long (say, one
simple case: output of printk printed to the console), those two
counters can reflect the issue.
2) Similarly in virtio net, recently I traced such counters the
current kernel does not have and it turned out that one of the output
queues in the backend behaves badly.
...

Stop/wake queue counters may not show directly the root cause of the
issue, but help us 'guess' to some extent.

I'm surprised you say you can detect stall-related issues with this.
I guess virtio doesn't have BQL support, which makes it special.

Yes, virtio-net has a legacy orphan mode, this is something that needs
to be dropped in the future. This would make BQL much more easier to
be implemented.

It's not that we can't implement BQL,

Well, I don't say we can't, I say it's not easy as we need to deal
with the switching between two modes[1]. If we just have one mode like
TX interrupt, we don't need to care about that.

quoted

it's that it does not seem to
be benefitial - has been discussed many times.

Virtio doesn't differ from other NIC too much, for example gve supports bql.

1) There's no numbers in [1]
2) We only benchmark vhost-net but not others, for example, vhost-user
and hardware implementations
3) We don't have interrupt coalescing in 2018 but now we have with DIM

Only works well with hardware virtio cards though.

Thanks

[1] https://lore.kernel.org/netdev/20181205225323.12555-1-mst@redhat.com/ (local)

So fundamentally, someone needs to show benefit and no serious
regressions to add BQL at this point. I doubt it's easily practical.
Hacks like wireless does to boost buffer sizes might be necessary.

quoted

quoted

quoted

Normal HW drivers with BQL almost never stop the queue by themselves.
I mean - if they do, and BQL is active, then the system is probably
misconfigured (queue is too short). This is what we use at Meta to
detect stalls in drivers with BQL:

https://lore.kernel.org/all/20240131102150.728960-3-leitao@debian.org/ (local)

Daniel, I think this may be a good enough excuse to add per-queue stats
to the netdev genl family, if you're up for that. LMK if you want more
info, otherwise I guess ethtool -S is fine for now.

Thanks