Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> We want to rate-limit MEMORY_DEVICE_SIZE_CHANGE events per device,
> otherwise we can lose some events for devices. As we might not always have
> a device id, add the qom-path to the event and use that to rate-limit
> per device.

There are actually two reasons for adding qom-path.  One, you need it to
fix the rate limiting.  But adding to an external interface just to fix
an internal issue would be questionable.  Fortunately, there's also two:
make the event useful regardless of whether the user gave it an ID.  If
you have to respin, consider working two into the commit message.

I'd split this patch into "add qom-path" and "fix rate limiting".
Suggestion, not demand.

> This was noticed by starting a VM with two virtio-mem devices that each
> have a requested size > 0. The Linux guest will initialize both devices
> in parallel, resulting in losing MEMORY_DEVICE_SIZE_CHANGE events for
> one of the devices.
>
> Fixes: 722a3c783ef4 ("virtio-pci: Send qapi events when the virtio-mem size 
> changes")
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michal Privoznik <mprivozn@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>
> Follow up of:
>     20210921102434.24273-1-david@redhat.com">https://lkml.kernel.org/r/20210921102434.24273-1-david@redhat.com
>
> v1 -> v2:
> - Add the qom-path and use that identifier to rate-limit per device
> - Rephrase subject/description
>
> ---
>  hw/virtio/virtio-mem-pci.c | 3 ++-
>  monitor/monitor.c          | 9 +++++++++
>  qapi/machine.json          | 5 ++++-
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
> index fa5395cd88..dd5085497f 100644
> --- a/hw/virtio/virtio-mem-pci.c
> +++ b/hw/virtio/virtio-mem-pci.c
> @@ -87,6 +87,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
> *notifier, void *data)
>      VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
>                                           size_change_notifier);
>      DeviceState *dev = DEVICE(pci_mem);
> +    const char * qom_path = object_get_canonical_path(OBJECT(dev));

No space after this *, please.

>      const uint64_t * const size_p = data;
>      const char *id = NULL;
>  
> @@ -94,7 +95,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
> *notifier, void *data)
>          id = g_strdup(dev->id);
>      }
>  
> -    qapi_event_send_memory_device_size_change(!!id, id, *size_p);
> +    qapi_event_send_memory_device_size_change(!!id, id, *size_p, qom_path);

Doesn't this leak @qom_path?

>  }
>  
>  static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 46a171bca6..21c7a68758 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -474,6 +474,10 @@ static unsigned int qapi_event_throttle_hash(const void 
> *key)
>          hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
>      }
>  
> +    if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
> +        hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
> +    }
> +
>      return hash;
>  }
>  
> @@ -496,6 +500,11 @@ static gboolean qapi_event_throttle_equal(const void *a, 
> const void *b)
>                         qdict_get_str(evb->data, "node-name"));
>      }
>  
> +    if (eva->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
> +        return !strcmp(qdict_get_str(eva->data, "qom-path"),
> +                       qdict_get_str(evb->data, "qom-path"));
> +    }
> +
>      return TRUE;
>  }
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..2487c92f18 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1245,8 +1245,11 @@
>  # action).
>  #
>  # @id: device's ID
> +#
>  # @size: the new size of memory that the device provides
>  #
> +# @qom-path: path to the device object in the QOM tree (since 6.2)
> +#
>  # Note: this event is rate-limited.
>  #
>  # Since: 5.1
> @@ -1259,7 +1262,7 @@
>  #
>  ##
>  { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> -  'data': { '*id': 'str', 'size': 'size' } }
> +  'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>  
>  
>  ##

With the two code remarks addressed:
Reviewed-by: Markus Armbruster <armbru@redhat.com>