Re: [Qemu-devel] [PATCH v2 0/2]vhost-user: Extend protocol to seek response for any command.

[Michael S. Tsirkin]

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" <address@hidden> wrote:
> 
> >On Fri, Jul 01, 2016 at 02:46:20AM -0700, Prerna Saxena wrote:
> >> From: Prerna Saxena <address@hidden>
> >> 
> >> 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