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

[Prerna Saxena]

Hi Marc,
Thank you for taking a look.




On 25/07/16 3:57 pm, "Marc-André Lureau" <address@hidden> wrote:

>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) {
>         ^~~~~~~~~~~~~~~

I really need to check on this. I am pretty positive I had verified this before 
posting, but its been a while since these patches were posted.


>
>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)

Right. There is a reason I havent reworded it here. We already have a 
VHOST_USER_REPLY_MASK 
flag that assumes that the incoming message is a reply to an already-sent vhost 
command.
Use of the word ‘REPLY’ in this context would have caused some confusion.

>
>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

Regards,
Prerna