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

[David Hildenbrand]

On 23.09.21 09:34, Markus Armbruster wrote:
> 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.

Sure, makes sense.

> > 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.

Whoops :)

> >       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?

I was asking myself the same question, but ended up essentially copying 
what hw/core/machine.c:machine_query_hotpluggable_cpus() does.

object_get_canonical_path() will end up doing a g_strdup(), just like we 
do with id here. I assume qapi code will end up freeing both strings, right?

[...]

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

Thanks, I'll respin!

--
Thanks,

David / dhildenb