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

Re: [Xen-devel] [PATCH RFC 0/6] Slotted channels for sync vm_events

On 2/12/19 7:01 PM, Tamas K Lengyel wrote:
On Thu, Feb 7, 2019 at 9:06 AM Petre Ovidiu PIRCALABU
<ppircalabu@xxxxxxxxxxxxxxx> wrote:

On Thu, 2019-02-07 at 11:46 +0000, George Dunlap wrote:
On 2/6/19 2:26 PM, Petre Ovidiu PIRCALABU wrote:
On Wed, 2018-12-19 at 20:52 +0200, Petre Pircalabu wrote:
This patchset is a rework of the "multi-page ring buffer" for
patch based on Andrew Cooper's comments.
For synchronous vm_events the ring waitqueue logic was
unnecessary as
vcpu sending the request was blocked until a response was
To simplify the request/response mechanism, an array of slotted
was created, one per vcpu. Each vcpu puts the request in the
corresponding slot and blocks until the response is received.

I'm sending this patch as a RFC because, while I'm still working
way to
measure the overall performance improvement, your feedback would
be a

Is anyone still using asynchronous vm_event requests? (the vcpu is
blocked and no response is expected).
If not, I suggest that the feature should be removed as it
(significantly) increases the complexity of the current

Could you describe in a bit more detail what the situation
is?  What's
the current state fo affairs with vm_events, what you're trying to
change, why async vm_events is more difficult?

The main reason for the vm_events modification was to improve the
overall performance in high throughput introspection scenarios. For
domus with a higher vcpu count, a vcpu could sleep for a certain period
of time while waiting for a ring slot to become available
The first patchset only increased the ring size, and the second
iteraton, based on Andrew Copper's comments, proposed a separate path
to handle synchronous events ( a slotted buffer for each vcpu) in order
to have the events handled independently of one another. To handle
asynchronous events, a dynamically allocated vm_event ring is used.
While the implementation is not exactly an exercise in simplicity, it
preserves all the needed functionality and offers fallback if the Linux
domain running the monitor application doesn't support
However, the problem got a little bit more complicated when I tried
implementing the vm_events using an IOREQ server (based on Paul
Durrant's comments). For synchronous vm_events, it simplified things a
little, eliminating both the need for a special structure to hold the
processing state and the evtchns for each vcpu.
The asynchronous events were a little more tricky to handle. The
buffered ioreqs were a good candidate, but the only thing usable is the
corresponding evtchn in conjunction with an existing ring. In order to
use them, a mock buffered ioreq should be created and transmitted, with
the only meaningful field being the ioreq type.

I certainly think it would be better if you could write the new
interface without having to spend a lot of effort supporting modes
you think nobody uses.

On the other hand, getting into the habit of breaking stuff, even for
people we don't know about, will be a hindrance to community growth;
commitment to keeping it working will be a benefit to growth.

But of course, we haven't declared the vm_event interface 'supported'
(it's not even mentioned in the SUPPORT.md document yet).

Just for the sake of discussion, would it be possible / reasonble,
instance, to create a new interface, vm_events2, instead?  Then you
could write it to share the ioreq interface without having legacy
baggage you're not using; we could deprecate and eventually remove
vm_events1, and if anyone shouts, we can put it back.


Yes, it's possible and it will GREATLY simplify the implementation. I
just have to make sure the interfaces are mutually exclusive.

I'm for removing features from the vm_event interface that are no
longer in use, especially if they block more advantageous changes like
this one. We don't know what the use-case was for async events nor
have seen anyone even mention them since I've been working with Xen.
Creating a new interface, as mentioned above, would make sense if
there was a disagreement with retiring this feature. I don't think
that's the case. I certainly would prefer not having to maintain two
separate interfaces going forward without a clear justification and
documented use-case explaining why we keep the old interface around.

AFAICT, the async model is broken conceptually as well, so it makes no sense. It would make sense, IMHO, if it would be lossy (i.e. we just write in the ring buffer, if somebody manages to "catch" an event while it flies by then so be it, if not it will be overwritten). If it would have been truly lossy, I'd see a use-case for it, for gathering statistics maybe.

However, as the code is now, the VCPUs are not paused only if there's still space in the ring buffer. If there's no more space in the ring buffer, the VCPU trying to put an event in still gets paused by the vm_event logic, which means that these events are only "half-async".

FWIW, I'm with Tamas on this one: if nobody cares about async events / comes forward with valid use cases or applications using them, I see no reason why we should have this extra code to maintain, find bugs in, and trip over other components (migration, in Andrew's example).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.