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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

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