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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

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
> > > > vm_events
> > > > patch based on Andrew Cooper's comments.
> > > > For synchronous vm_events the ring waitqueue logic was
> > > > unnecessary as
> > > > the
> > > > vcpu sending the request was blocked until a response was
> > > > received.
> > > > To simplify the request/response mechanism, an array of slotted
> > > > channels
> > > > 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
> > > > on
> > > > way to
> > > > measure the overall performance improvement, your feedback would
> > > > be a
> > > > great
> > > > assistance.
> > > >
> > >
> > > Is anyone still using asynchronous vm_event requests? (the vcpu is
> > > not
> > > 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
> > > implementation.
> >
> > 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
> (__vm_event_claim_slot)
> 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
> IOCTL_PRIVCMD_MMAP_RESOURCE.
> 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
> > vm_event
> > interface without having to spend a lot of effort supporting modes
> > that
> > 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;
> > a
> > 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,
> > for
> > 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.
> >
> > Thoughts?
> >
> >  -George
> 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.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel