On 23.09.21 09:34, Markus Armbruster wrote:
David Hildenbrand <[email protected]> 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 <[email protected]>
Cc: "Dr. David Alan Gilbert" <[email protected]>
Cc: Markus Armbruster <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Eric Blake <[email protected]>
Cc: Igor Mammedov <[email protected]>
Cc: Michal Privoznik <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
Follow up of:
https://lkml.kernel.org/r/[email protected]
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 <[email protected]>
Thanks, I'll respin!
--
Thanks,
David / dhildenb
