Re: [Qemu-devel] [PATCH v4 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Michael S. Tsirkin]

On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote:
> 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.

I don't see confusion, I think I agree with Marc André.


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