Re: [PATCH v5] [for-10.1] virtio: add VIRTQUEUE_ERROR QAPI event

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 04.04.25 09:46, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> For now we only log the vhost device error, when virtqueue is actually
>>> stopped. Let's add a QAPI event, which makes possible:
>>>
>>>   - collect statistics of such errors
>>>   - make immediate actions: take core dumps or do some other debugging
>>>   - inform the user through a management API or UI, so that (s)he can
>>>    react somehow, e.g. reset the device driver in the guest or even
>>>    build up some automation to do so
>>>
>>> Note that basically every inconsistency discovered during virtqueue
>>> processing results in a silent virtqueue stop.  The guest then just
>>> sees the requests getting stuck somewhere in the device for no visible
>>> reason.  This event provides a means to inform the management layer of
>>> this situation in a timely fashion.
>>>
>>> The event could be reused for some other virtqueue problems (not only
>>> for vhost devices) in future. For this it gets a generic name and
>>> structure.
>>>
>>> We keep original VHOST_OPS_DEBUG(), to keep original debug output as is
>>> here, it's not the only call to VHOST_OPS_DEBUG in the file.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Reviewed-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
>>> ---
>>>
>>> v5: resend, update version in QAPI to 10.1
>>>     drop a-b by Markus (too much time passed, the context could
>>>     changed. Markus, is the patch still OK?)
>> 
>> Happy to take another look :)
>> 
>>>  hw/virtio/vhost.c | 12 +++++++++---
>>>  monitor/monitor.c | 10 ++++++++++
>>>  qapi/qdev.json    | 32 ++++++++++++++++++++++++++++++++
>>>  3 files changed, 51 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 6aa72fd434..0b205cef73 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -15,6 +15,7 @@
>>>   
>>>  #include "qemu/osdep.h"
>>>  #include "qapi/error.h"
>>> +#include "qapi/qapi-events-qdev.h"
>>>  #include "hw/virtio/vhost.h"
>>>  #include "qemu/atomic.h"
>>>  #include "qemu/range.h"
>>> @@ -1442,11 +1443,16 @@ static void vhost_virtqueue_error_notifier(EventNotifier *n)
>>>      struct vhost_virtqueue *vq = container_of(n, struct vhost_virtqueue,
>>>                                                error_notifier);
>>>      struct vhost_dev *dev = vq->dev;
>>> -    int index = vq - dev->vqs;
>>>  
>>>      if (event_notifier_test_and_clear(n) && dev->vdev) {
>>> -        VHOST_OPS_DEBUG(-EINVAL,  "vhost vring error in virtqueue %d",
>>> -                        dev->vq_index + index);
>>> +        int ind = vq - dev->vqs + dev->vq_index;
>>> +        DeviceState *ds = &dev->vdev->parent_obj;
>>> +
>>> +        VHOST_OPS_DEBUG(-EINVAL,  "vhost vring error in virtqueue %d", ind);
>>> +        qapi_event_send_virtqueue_error(ds->id, ds->canonical_path, ind,
>>> +                                        VIRTQUEUE_ERROR_VHOST_VRING_ERROR,
>>> +                                        "vhost reported failure through vring "
>>> +                                        "error fd");
>>>      }
>>>  }
>>>   
>>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>>> index c5a5d30877..1296a9207e 100644
>>> --- a/monitor/monitor.c
>>> +++ b/monitor/monitor.c
>>> @@ -313,6 +313,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>>>      [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
>>>      [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
>>>      [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
>>> +    [QAPI_EVENT_VIRTQUEUE_ERROR]   = { 1000 * SCALE_MS },
>>>      [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>>>      [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>>>      [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
>>> @@ -499,6 +500,10 @@ static unsigned int qapi_event_throttle_hash(const void *key)
>>>          hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>>>      }
>>>   
>>> +    if (evstate->event == QAPI_EVENT_VIRTQUEUE_ERROR) {
>>> +        hash += g_str_hash(qdict_get_str(evstate->data, "device"));
>> 
>> Isn't @device optional?  Should you use @qom-path instead?
>
> Yes, I'm thinking about it too.

This function and the next one together enable fine-grained event
throttling.  I figure the intent of your patch is to rate-limit this
event per device.  For that, you must use qom-path.  If you wanted to
rate-limit per virtqueue, you'd have to throw in @virtqueue.

As is, you have a crash bugL g_str_hash() expects a non-null argument.

>                                 Maybe, drop the "device" field from the event at all, and keep only qom-path? Years passed since movement to use qom paths..

Possible justifications for having @device: consistency with other
events, convenience for users.  I can't judge the latter.

>
>> 
>>> +    }
>>> +
>>>      return hash;
>>>  }
>>>   
>>> @@ -527,6 +532,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
>>>                         qdict_get_str(evb->data, "qom-path"));
>>>      }
>>>   
>>> +    if (eva->event == QAPI_EVENT_VIRTQUEUE_ERROR) {
>>> +        return !strcmp(qdict_get_str(eva->data, "device"),
>>> +                       qdict_get_str(evb->data, "device"));
>> 
>> Likewise.
>> 
>>> +    }
>>> +
>>>      return TRUE;
>>>  }
>>>   
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index 25cbcf977b..2d20f4777e 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -187,3 +187,35 @@
>>>  { 'command': 'device-sync-config',
>>>    'features': [ 'unstable' ],
>>>    'data': {'id': 'str'} }
>>> +
>>> +##
>>> +# @VirtqueueError:
>>> +#
>>> +# @vhost-vring-error: Vhost device reported failure through
>>> +#     through vring error fd.
>> 
>> One "through" too many.
>> 
>> I'm not quite sure I understand what you're trying to express.  Is it
>> "the vhost device has communicated failure via the vring error file
>> descriptor"?
>
> Yes. Will use your wording.
>
>> 
>> I know next to nothing about vhost devices...
>> 
>>> +#
>>> +# Since: 10.1
>>> +##
>>> +{ 'enum': 'VirtqueueError',
>>> +  'data': [ 'vhost-vring-error' ] }
>>> +
>>> +##
>>> +# @VIRTQUEUE_ERROR:
>>> +#
>>> +# Emitted when a device virtqueue fails in runtime.
>> 
>> I think it's "at runtime".
>> 
>> Can a device have more than one virtqueue?
>
> Yes. And event has "virtqueue": "int" for it.

Got it, thanks!

>>> +#
>>> +# @device: the device's ID if it has one
>>> +#
>>> +# @path: the device's QOM path
>> 
>> I agree with your follow-up: name it @qom-path.
>> 
>>> +#
>>> +# @virtqueue: virtqueue index
>> 
>> Bear with me...  What's a virtqueue index?
>
> sequence number of virtqueue of the vhost device

Maybe something like "the index of the virtqueue that failed".

>>> +#
>>> +# @error: error identifier
>>> +#
>>> +# @description: human readable description
>>> +#
>>> +# Since: 10.1
>>> +##
>>> +{ 'event': 'VIRTQUEUE_ERROR',
>>> + 'data': { '*device': 'str', 'path': 'str', 'virtqueue': 'int',
>>> +            'error': 'VirtqueueError', 'description': 'str'} }
>>