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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Mar 4, 2019 at 9:01 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> On 2/19/19 11:48 AM, Razvan Cojocaru wrote:
> > 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
> >>>>>> 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.
> >
> > 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).
>
> The whole idea would be that we *don't* maintain "v1", unless someone
> shows up and says they want to use it.
>
> To let you know where I'm coming from:  It's actually fairly uncommon
> for people using something to participate in the community that
> generates it.  For example, a few years ago I spent some time with a
> GCoC student making libxl bindings for Golang.  In spite of the fact
> that they're still at 'early prototype' stage, apparently there's a
> community of people that have picked those up and are using them.
>
> Such people often:
> * Start using without getting involved with the community
> * Don't read the mailing list to hear what's going to happen
> * Won't complain if something breaks, but will just go find a different
> platform.
>
> If you have something that already works, switching to something else is
> a huge effort; if what you used to have is broken and you have to switch
> everything over anyway, then you're much more likely to try something else.
>
> There are many reason for the success of both Linux and Linux
> containers, but one major one is the fairly fanatical commitment they
> have to backwards compatibility and avoiding breaking userspace
> applications.  By contrast, it seems to me that the xend -> xl
> transition, while necessary, has been an inflection point that caused a
> lot of people to consider moving away from Xen (since a lot of their old
> tooling stopped working).
>
> So; at the moment, we don't know if anyone's using the async
> functionality or not.  We have three choices:
>
> A. Try to implement both sync / async in the existing interface.
> B. Re-write the current interface to be sync-only.
> C. Create a new interface ('v2') from scratch, deleting 'v1' when 'v2'
> is ready.
>
> Option A *probaby* keeps silent users.  But, Petre spends a lot of time
> designing around and debugging something that he doesn't use or care
> about, and that he's not sure *anyone* uses.  Not great.
>
> Option B is more efficient for Petre.  But if there are users, any ones
> that don't complain we lose immediately.  If any users do complain, then
> again we have the choice: Try to retrofit the async functionality, or
> tell them we're sorry, they'll have to port their thing to v2 or go away.
>
> In the case of option C, we can leave 'v1' there as long as we want.  If
> we delete it, and people complain, it won't be terribly difficult to
> reinstate 'v1' without affecting 'v2'.
>
> I mean, a part of me completely agrees with you: get rid of cruft
> nobody's using; if you want to depend on something someone else is
> developing, at least show up and get involved in the community, or don't
> complain when it goes away.
>
> But another part of me is just worried the long-term effects of this
> kind of behavior.
>
> Anyway, I won't insist on having a v2 if nobody else says anything; I
> just wanted to make sure the "invisible" effects got proper consideration.

I agree with the not-break-things for convenience only for stable
interfaces. This is not a stable interface however and never was -
it's experimental at this point. So we should treat it as such and not
keep dead code around just to make it appear to be a stable interface.

Tamas

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