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

[Prerna Saxena]

Hi Marc-Andre,
Thank you for taking a look.





On 03/07/16 5:17 pm, "Marc-André Lureau" <address@hidden> wrote:

>Hi
>
>On Fri, Jul 1, 2016 at 11:46 AM, Prerna Saxena <address@hidden> 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.
>>
>> 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.
>>
>
>Overall, that looks good to me.
>
>Why do we have both "response" and "reply" which basically means the
>same thing, right? I would rather stick with "reply".

Allright, will rename this function to process_message_reply().

>
>I am not convinced the first patch is needed, imho it is a
>workaround/hack, the solution is given with the patch 2 only.

Great, I’ll post a v3 with just Patch2.

Regards,
Prerna

>
>> 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
>>
>
>
>
>-- 
>Marc-André Lureau
>