"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * David Hildenbrand (da...@redhat.com) wrote:
>> 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.
>>
>> 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 <arm...@redhat.com>
>> Cc: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>> Cc: Markus Armbruster <arm...@redhat.com>
>> Cc: Michael S. Tsirkin <m...@redhat.com>
>> Cc: Eric Blake <ebl...@redhat.com>
>> Cc: Igor Mammedov <imamm...@redhat.com>
>> Cc: Michal Privoznik <mpriv...@redhat.com>
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>>
>> Follow up of:
>> https://lkml.kernel.org/r/20210921102434.24273-1-da...@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));
>> 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);
>> }
>>
>> 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"));
>> + }
>
> It makes me wonder if all these entries could turn into:
> str = qdict_get_try_str(qdict, "qom-path");
> if (str) {
> hash += g_str_hash(str);
> }
>
> and then stop worrying about checking each eventtype there?
Prone to accidental capture when we add to event data later on. I feel
it's better to be explicit.
