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

[Prerna Saxena]

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.

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