On Mon, Jul 04, 2016 at 04:50:04PM +0000, Prerna Saxena wrote: > Hi Michael, > Thank you for taking a look. > > > > > > On 04/07/16 5:29 pm, "Michael S. Tsirkin" <m...@redhat.com> wrote: > > >On Fri, Jul 01, 2016 at 02:46:20AM -0700, Prerna Saxena wrote: > >> From: Prerna Saxena <prerna.sax...@nutanix.com> > >> > >> The current vhost-user protocol requires the client to send responses to > >> only a > >> few commands. For the remaining commands, it is impossible for QEMU to > >> know the > >> status of the requested operation -- ie, did it succeed? If so, by what > >> time? > >> > >> This is inconvenient, and can also lead to races. As an example: > >> > >> (1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net > >> application). > >> Note that SET_MEM_TABLE does not require a reply according to the spec. > >> (2) Qemu commits the memory to the guest. > >> (3) Guest issues an I/O operation over a new memory region which was > >> configured on (1). > >> (4) The application hasn't yet remapped the memory, but it sees the I/O > >> request. > >> (5) The application cannot satisfy the request because it does not know > >> about those GPAs. > >> > >> Note that the kernel implementation does not suffer from this limitation > >> since messages are sent via an ioctl(). The ioctl() blocks until the > >> backend (eg. vhost-net) completes the command and returns (with an error > >> code). > >> > >> Changing the behaviour of current vhost-user commands would break existing > >> applications. > >> To work around this race, Patch 1 adds a get_features command to be sent > >> before returning from set_mem_table. While this is not a complete fix, it > >> will help client applications that strictly process messages in order. > >> > >> The second patch introduces a protocol extension, > >> VHOST_USER_PROTOCOL_F_REPLY_ACK. This feature, if negotiated, allows QEMU > >> to request a response to any message by setting the newly introduced > >> "need_response" flag. The application must then respond to qemu by > >> providing a status about the requested operation. > > > > > >OK this all looks very reasonable (and I do like patch 1 too) > >but there's one source of waste here: we do not need to > >synchronize when we set up device the first time > >when hdev->memory_changed is false. > > > >I think we should test that and skip synch in both patches > >unless hdev->memory_changed is set. > > I do not entirely agree with that. The first set_mem_table command is not > much different from subsequent set_mem_table calls. > For all cases, there is a fair chance that the vhost-user application may, > for some reason, not be able to map the guest memory. > This protocol extension provides a mechanism for such errors to be propagated > back to QEMU. It is upto QEMU to acknowledge the failure (by terminating > itself or failing the device) or ignore it. However, in the absence of such a > mechanism, it would be really bad for QEMU to believe that the vhost > application is all set to process guest requests when reality is quite the > opposite.
Same applies to any other request in theory. Backend can disconnect if it can not handle a request, this seems sufficient to me. > Also, as pointed out before, QEMU needs to have a notion of _when_ the memory > mapping was finished, so that it may proceed to pass on actual requests to > the vhost user application. The race described in the covering letter (above) > can potentially happen even at first-time initialization. > > This protocol extension is an attempt to bridge the subtle behavioural > difference between vhost-user and vhost-kernel. Patch 1, in my opinion, makes > the code less intuitive. This is because we are calling a GET_FEATURES vhost > message from inside the handler for another vhost command— SET_MEM_TABLE. > However, if you think it better to have both Patch 1 & 2, I’ll be happy to > post both. > > Regards, > Prerna > > > > > > >> Changelog: > >> --------- > >> Changes since v1: > >> Patch 1 : Ask for get_features before returning from set_mem_table(new). > >> Patch 2 : * Improve documentation. > >> * Abstract out commonly used operations in the form of a > >> function, process_message_response(). Also implement this only for > >> SET_MEM_TABLE. > >> > >> Prerna Saxena (2): > >> vhost-user: Attempt to prevent a race on set_mem_table. > >> vhost-user : Introduce a new feature VHOST_USER_PROTOCOL_F_REPLY_ACK. > >> > >> docs/specs/vhost-user.txt | 40 ++++++++++++ > >> hw/virtio/vhost-user.c | 157 > >> ++++++++++++++++++++++++++++++++-------------- > >> 2 files changed, 150 insertions(+), 47 deletions(-) > >> > >> -- > >> 1.8.1.2
