On 22.01.24 18:52, Hanna Czenczek wrote:
On 22.01.24 18:41, Hanna Czenczek wrote:
On 05.01.24 15:30, Fiona Ebner wrote:
Am 05.01.24 um 14:43 schrieb Fiona Ebner:
Am 03.01.24 um 14:35 schrieb Paolo Bonzini:
On 1/3/24 12:40, Fiona Ebner wrote:
I'm happy to report that I cannot reproduce the CPU-usage-spike
issue
with the patch, but I did run into an assertion failure when
trying to
verify that it fixes my original stuck-guest-IO issue. See below
for the
backtrace [0]. Hanna wrote in
https://issues.redhat.com/browse/RHEL-3934
I think it’s sufficient to simply call virtio_queue_notify_vq(vq)
after the virtio_queue_aio_attach_host_notifier(vq, ctx) call,
because
both virtio-scsi’s and virtio-blk’s .handle_output()
implementations
acquire the device’s context, so this should be directly
callable from
any context.
I guess this is not true anymore now that the AioContext locking was
removed?
Good point and, in fact, even before it was much safer to use
virtio_queue_notify() instead. Not only does it use the event
notifier
handler, but it also calls it in the right thread/AioContext just by
doing event_notifier_set().
But with virtio_queue_notify() using the event notifier, the
CPU-usage-spike issue is present:
Back to the CPU-usage-spike issue: I experimented around and it
doesn't
seem to matter whether I notify the virt queue before or after
attaching
the notifiers. But there's another functional difference. My patch
called virtio_queue_notify() which contains this block:
if (vq->host_notifier_enabled) {
event_notifier_set(&vq->host_notifier);
} else if (vq->handle_output) {
vq->handle_output(vdev, vq);
In my testing, the first branch was taken, calling
event_notifier_set().
Hanna's patch uses virtio_queue_notify_vq() and there,
vq->handle_output() will be called. That seems to be the relevant
difference regarding the CPU-usage-spike issue.
I should mention that this is with a VirtIO SCSI disk. I also
attempted
to reproduce the CPU-usage-spike issue with a VirtIO block disk, but
didn't manage yet.
What I noticed is that in virtio_queue_host_notifier_aio_poll(),
one of
the queues (but only one) will always show as nonempty. And then,
run_poll_handlers_once() will always detect progress which explains
the
CPU usage.
The following shows
1. vq address
2. number of times vq was passed to
virtio_queue_host_notifier_aio_poll()
3. number of times the result of virtio_queue_host_notifier_aio_poll()
was true for the vq
0x555fd93f9c80 17162000 0
0x555fd93f9e48 17162000 6
0x555fd93f9ee0 17162000 0
0x555fd93f9d18 17162000 17162000
0x555fd93f9db0 17162000 0
0x555fd93f9f78 17162000 0
And for the problematic one, the reason it is seen as nonempty is:
0x555fd93f9d18 shadow_avail_idx 8 last_avail_idx 0
vring_avail_idx(vq) also gives 8 here. This is the vs->event_vq and
s->events_dropped is false in my testing, so
virtio_scsi_handle_event_vq() doesn't do anything.
Those values stay like this while the call counts above increase.
So something going wrong with the indices when the event notifier
is set
from QEMU side (in the main thread)?
The guest is Debian 12 with a 6.1 kernel.
So, trying to figure out a new RFC version:
About the stack trace you, Fiona, posted: As far as I understand,
that happens because virtio_blk_drained_end() calling
virtio_queue_notify_vq() wasn’t safe after all, and instead we need
to use virtio_queue_notify(). Right?
However, you say using virtio_queue_notify() instead causes busy
loops of doing nothing in virtio-scsi (what you describe above). I
mean, better than a crash, but, you know. :)
I don’t know have any prior knowledge about the event handling,
unfortunately. The fact that 8 buffers are available but we don’t
use any sounds OK to me; as far as I understand, we only use those
buffers if we have any events to push into them, so as long as we
don’t, we won’t. Question is, should we not have its poll handler
return false if we don’t have any events (i.e. events_dropped is
false)? Would that solve it?
Or actually, maybe we could just skip the virtio_queue_notify() call
for the event vq? I.e. have it be `if (vq !=
VIRTIO_SCSI_COMMON(s)->event_vq) { virtio_queue_notify(vdev, i); }`?
I wouldn’t like that very much, (a) because this would make it
slightly cumbersome to put that into
virtio_queue_aio_attach_host_notifier*(), and (b) in case that does
fix it, I do kind of feel like the real problem is that we use
virtio_queue_host_notifier_aio_poll() for the event vq, which tells
the polling code to poll whenever the vq is non-empty, but we (AFAIU)
expect the event vq to be non-empty all the time.
Turns out there was commit 38738f7dbbda90fbc161757b7f4be35b52205552
(“virtio-scsi: don't waste CPU polling the event virtqueue”) by Stefan,
which specifically intended to not use
virtio_queue_host_notifier_aio_poll() for the event vq. I think the
problem is that virtio_scsi_drained_end() should have taken care to use
virtio_queue_aio_attach_host_notifier_no_poll() for the event vq.
If we do that, I think running virtio_queue_notify() on the event vq too
should be reasonable. We still want to check whether there are new
buffers available in case we have events_dropped. I don’t know whether
it’s really necessary, but without virtio_queue_host_notifier_aio_poll()
installed, it shouldn’t hurt.
Hanna
