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

David Hildenbrand posted 1 patch 2 years, 7 months ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210922125734.79712-1-david@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Markus Armbruster <armbru@redhat.com>, David Hildenbrand <david@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eric Blake <eblake@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>
There is a newer version of this series
hw/virtio/virtio-mem-pci.c | 3 ++-
monitor/monitor.c          | 9 +++++++++
qapi/machine.json          | 5 ++++-
3 files changed, 15 insertions(+), 2 deletions(-)
[PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by David Hildenbrand 2 years, 7 months ago
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 <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:
    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));
     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"));
+    }
+
     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'} }
 
 
 ##
-- 
2.31.1


Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by Dr. David Alan Gilbert 2 years, 7 months ago
* David Hildenbrand (david@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 <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:
>     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));
>      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?

Dave

> +
>      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'} }
>  
>  
>  ##
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by Markus Armbruster 2 years, 7 months ago
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * David Hildenbrand (david@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 <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:
>>     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));
>>      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.


Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by Markus Armbruster 2 years, 7 months ago
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:
>     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>


Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by David Hildenbrand 2 years, 7 months ago
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:
>>      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


Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
Posted by Paolo Bonzini 2 years, 7 months ago
On 22/09/21 14:57, David Hildenbrand 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 <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>
> ---

I cannot find v3 in my inbox, but I queued it from patchew.

Thanks,

Paolo

> 
> Follow up of:
>      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));
>       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"));
> +    }
> +
>       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'} }
>   
>   
>   ##
>