Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler

[Hanna Czenczek <hreitz@redhat.com>]

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.

Hanna