[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/2] vhost-user: Introduce a new protocol fea
From: |
Prerna Saxena |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK. |
Date: |
Wed, 27 Jul 2016 12:56:18 +0000 |
Hi Marc,
Thanks, please find my reply inline.
On 27/07/16 4:35 pm, "Marc-André Lureau" <address@hidden> wrote:
>Hi
>
>On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <address@hidden> wrote:
>> From: Prerna Saxena <address@hidden>
>>
>> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>>
>> If negotiated, client applications should send a u64 payload in
>> response to any message that contains the "need_response" bit set
>> on the message flags. Setting the payload to "zero" indicates the
>> command finished successfully. Likewise, setting it to "non-zero"
>> indicates an error.
>>
>> Currently implemented only for SET_MEM_TABLE.
>>
>> Signed-off-by: Prerna Saxena <address@hidden>
>> ---
>> docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
>> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++++++
>> 2 files changed, 73 insertions(+)
>>
>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> index 777c49c..57df586 100644
>> --- a/docs/specs/vhost-user.txt
>> +++ b/docs/specs/vhost-user.txt
>> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>> * Flags: 32-bit bit field:
>> - Lower 2 bits are the version (currently 0x01)
>> - Bit 2 is the reply flag - needs to be sent on each reply from the slave
>> + - Bit 3 is the need_response flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK
>> for
>> + details.
>
>Why need_response and not "need reply"?
(I’d already pointed this out earlier, but looks like I was possibly not very
clear.)
Before deciding on the right name for Bit 3, let us see the nomenclature for
Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from
the slave”.
So we already have a _reply_ flag in use. If the name Bit 3 as the _need_reply_
flag, don’t you think it would be ultra-confusing ? I found it confusing when
I reviewed the documentation with this different term.
So I chose the name need_response with much deliberation — it conveys the
essence of what this flag means to achieve, but without adding to confusion.
>
>btw, I wonder if it would be worth to introduce an enum at this point
>
>> * Size - 32-bit size of the payload
>>
>>
>> @@ -126,6 +128,8 @@ the ones that do:
>> * VHOST_GET_VRING_BASE
>> * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
>>
>> +[ Also see the section on REPLY_ACK protocol extension. ]
>> +
>> There are several messages that the master sends with file descriptors
>> passed
>> in the ancillary data:
>>
>> @@ -254,6 +258,7 @@ Protocol features
>> #define VHOST_USER_PROTOCOL_F_MQ 0
>> #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
>> #define VHOST_USER_PROTOCOL_F_RARP 2
>> +#define VHOST_USER_PROTOCOL_F_REPLY_ACK 3
>>
>> Message types
>> -------------
>> @@ -464,3 +469,39 @@ Message types
>> is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>> The first 6 bytes of the payload contain the mac address of the guest
>> to
>> allow the vhost user backend to construct and broadcast the fake RARP.
>> +
>> +VHOST_USER_PROTOCOL_F_REPLY_ACK:
>> +-------------------------------
>> +The original vhost-user specification only demands responses for certain
>
>responses/replies
If you feel strongly about it, will change it here.
>
>> +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_response" [Bit 3] flag to any command. This indicates that
>
>need reply, you can remove the "newly introduced" (it's not going to
>be so new after a while)
* need_reply = no I don’t agree, for reasons cited earlier.
* remove the “newly introduced” phrase = agree, will do.
>
>> +the client MUST respond with a Payload VhostUserMsg indicating success or
>
>I would put right here for clarity:
>
>...MUST respond with a Payload VhostUserMsg (unless the message has
>already an explicit reply body)...
>
>alternatively, I would forbid using the bit 3 on commands that have
>already an explicit reply.
I don’t currently have any code that raises an error for such cases.
The implementation silently ignores it.
>
>> +failure. The payload should be set to zero on success or non-zero on
>> failure.
>> +In other words, response 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 as per the original vhost-user protocol, the following four
>> messages
>> +anyway require distinct responses from the vhost-user client process:
>
>I don't think we need to repeat this list (already redundant with the
>list in "Communication" part, and with the message specification, 2
>times is enough imho)
Ok, will remove it for brevity.
>
>> + * VHOST_GET_FEATURES
>> + * VHOST_GET_PROTOCOL_FEATURES
>> + * VHOST_GET_VRING_BASE
>> + * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
>> +
>> +For these message types, the presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or
>> +need_response bit being set brings no behaviourial change.
>
>Reply
Again, I disagree, for reasons cited above.
[..snip..] removing the rest.
>
>
>
>--
>Marc-André Lureau
Thanks once again for the quick review.
Let me know if this makes sense, so I’ll quickly spin a cleaned-up patch which
includes the tiny documentation changes.
Regards,
Prerna
[Qemu-devel] [PATCH v4 2/2] vhost-user: Attempt to fix a race with set_mem_table., Prerna Saxena, 2016/07/27