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

[Michael S. Tsirkin]

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.


> 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