All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
@ 2021-09-22 12:57 David Hildenbrand
  2021-09-22 15:39 ` Dr. David Alan Gilbert
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-09-22 12:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: Michael S . Tsirkin, Michal Privoznik, David Hildenbrand,
	Markus Armbruster, Dr. David Alan Gilbert, Igor Mammedov,
	Eric Blake

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
  2021-09-22 12:57 [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device David Hildenbrand
@ 2021-09-22 15:39 ` Dr. David Alan Gilbert
  2021-09-23  7:36   ` Markus Armbruster
  2021-09-23  7:34 ` Markus Armbruster
  2021-10-02  9:56 ` Paolo Bonzini
  2 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2021-09-22 15:39 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Michael S . Tsirkin, Michal Privoznik, qemu-devel,
	Markus Armbruster, Igor Mammedov, Eric Blake

* 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
  2021-09-22 12:57 [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device David Hildenbrand
  2021-09-22 15:39 ` Dr. David Alan Gilbert
@ 2021-09-23  7:34 ` Markus Armbruster
  2021-09-23  8:14   ` David Hildenbrand
  2021-10-02  9:56 ` Paolo Bonzini
  2 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2021-09-23  7:34 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Michael S . Tsirkin, Michal Privoznik, qemu-devel,
	Dr. David Alan Gilbert, Igor Mammedov, Eric Blake

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>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
  2021-09-22 15:39 ` Dr. David Alan Gilbert
@ 2021-09-23  7:36   ` Markus Armbruster
  0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2021-09-23  7:36 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Michael S . Tsirkin, Michal Privoznik, David Hildenbrand,
	qemu-devel, Igor Mammedov, Eric Blake

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
  2021-09-23  7:34 ` Markus Armbruster
@ 2021-09-23  8:14   ` David Hildenbrand
  0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-09-23  8:14 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Michael S . Tsirkin, Michal Privoznik, qemu-devel,
	Dr. David Alan Gilbert, Igor Mammedov, Eric Blake

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device
  2021-09-22 12:57 [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device David Hildenbrand
  2021-09-22 15:39 ` Dr. David Alan Gilbert
  2021-09-23  7:34 ` Markus Armbruster
@ 2021-10-02  9:56 ` Paolo Bonzini
  2 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-10-02  9:56 UTC (permalink / raw)
  To: David Hildenbrand, qemu-devel
  Cc: Michael S . Tsirkin, Michal Privoznik, Markus Armbruster,
	Dr. David Alan Gilbert, Igor Mammedov, Eric Blake

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'} }
>   
>   
>   ##
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-10-02  9:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 12:57 [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device David Hildenbrand
2021-09-22 15:39 ` Dr. David Alan Gilbert
2021-09-23  7:36   ` Markus Armbruster
2021-09-23  7:34 ` Markus Armbruster
2021-09-23  8:14   ` David Hildenbrand
2021-10-02  9:56 ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.