Re: [Qemu-devel] [PATCH v3 0/2] vhost-user: Extend protocol to receive replies on any command.

[Marc-André Lureau]

Hi

On Mon, Jul 25, 2016 at 10:41 AM, Prerna <address@hidden> wrote:
>
>
> On Thu, Jul 7, 2016 at 12:04 PM, 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:
>>  [..snip..]
>>
>> References:
>> v1 : https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07152.html
>> v2 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00048.html
>>
>>
>> Prerna Saxena (2):
>>   vhost-user: Introduce a new protocol feature REPLY_ACK.
>>   vhost-user: Attempt to fix a race with set_mem_table.
>>
>>  docs/specs/vhost-user.txt |  44 +++++++++++++++
>>  hw/virtio/vhost-user.c    | 133
>> ++++++++++++++++++++++++++++++----------------
>>  2 files changed, 130 insertions(+), 47 deletions(-)
>>
>
> Ping !
> Michael, MarcAndre, Did you have a chance to look at this patch series?
>

That's not going to make it in 2.7 I am afraid. Beside the second
patch that I think is somewhat superflous or worse, as I said in
previous review (so I won't ack it, but Michael liked it and he is the
maintainer)

It fails to compile, easy to fix by moving process_message_reply after
vhost_user_read:

/home/elmarco/src/qemu/hw/virtio/vhost-user.c: In function
‘process_message_reply’:
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: warning: implicit
declaration of function ‘vhost_user_read’
[-Wimplicit-function-declaration]
     if (vhost_user_read(dev, &msg) < 0) {
         ^~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:5: warning: nested
extern declaration of ‘vhost_user_read’ [-Wnested-externs]
     if (vhost_user_read(dev, &msg) < 0) {
     ^~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c: At top level:
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:136:12: error: static
declaration of ‘vhost_user_read’ follows non-static declaration
 static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
            ^~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/virtio/vhost-user.c:117:9: note: previous
implicit declaration of ‘vhost_user_read’ was here
     if (vhost_user_read(dev, &msg) < 0) {
         ^~~~~~~~~~~~~~~

Secondly, make check just hangs in /x86_64/vhost-user/read-guest-mem
(a sign that backward compatibility is broken).

There is still many "response" wording, where "reply" should be used
for more consistency (VHOST_USER_NEED_RESPONSE_MASK and in the doc)

Regarding the doc, I would simplify it a bit:

VHOST_USER_PROTOCOL_F_REPLY_ACK:
-------------------------------
The original vhost-user specification only demands replies for certain
commands. This differs from the vhost protocol implementation where commands
are sent over an ioctl() call and block until the client has completed.

With this protocol extension negotiated, the sender (QEMU) can set the newly
introduced "need_reply" [Bit 3] flag to any command. This indicates that
the client MUST reply with a Payload VhostUserMsg indicating success or
failure. The payload should be set to zero on success or non-zero on failure.
In other words, reply message must be in the following format :

------------------------------------
| request | flags | size | payload |
------------------------------------

 * Request: 32-bit type of the request
 * Flags: 32-bit bit field:
 * Size: size of the payload ( see below)
 * Payload : a u64 integer, where a non-zero value indicates a failure.

This indicates to QEMU that the requested operation has
deterministically been met or not. Today, QEMU is expected to terminate
the main vhost-user loop upon receiving such errors. In future, qemu could
be taught to be more resilient for selective requests.

Note that for messages that already require distinct replies, the presence of
need_reply bit being set brings no behavioural change.

-- 
Marc-André Lureau