qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1] monitor: Consider "id" when rate-limiting MEMORY_DEVICE_S


From: David Hildenbrand
Subject: Re: [PATCH v1] monitor: Consider "id" when rate-limiting MEMORY_DEVICE_SIZE_CHANGE qapi events
Date: Wed, 22 Sep 2021 14:45:36 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 22.09.21 14:20, David Hildenbrand wrote:
On 22.09.21 14:11, Markus Armbruster wrote:
David Hildenbrand <david@redhat.com> writes:

We have to consider the device id, otherwise we'll lose some events for
unrelated devices. If the device does not have a device id (very unlikely),
the target of the notifications has to update the size of all devices
manually either way.

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.

Fascinating.

Event rate limiting works as follows.

An event is rate-limited when monitor_qapi_event_conf[event].rate != 0.

When such an event arrives, it is held in a bucket until a timer
associated with the bucket expires.  Putting an event in an empty bucket
starts its timer.  Putting an event in a non-empty bucket replaces its
old contents.

The bucket to use for an event depends on its event type, and for some
events also on certain event arguments.

This patch solves the "MEMORY_DEVICE_SIZE_CHANGE events from different
devices eat each other" by splitting the event's bucket.

Right, that's how it's getting used in libvirt where we noticed it.


The split is imperfect: each device with a qdev ID gets its own bucket,
all devices without ID have to share a bucket.

Yes, it's far from perfect. Fortunately upper layers (libvirt) barely do
that.


This is actually a flaw in the event's design: you can't distinguish
events from different devices without IDs.

To fix that flaw, add the QOM path to the event.

So the idea would be to extend the event by an optional QOM path
(because it's an existing event), but always setting it internally?


Thinking about it, not optional, because we always have a QOM path and extending an event with something not optional should just work AFAIKT.

Just implemented it, seems to work just fine. Will send a v2 -- thanks!

--
Thanks,

David / dhildenb




reply via email to

[Prev in Thread] Current Thread [Next in Thread]