|
[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 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? -GeorgeYes, 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). Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |