Hi Marc, Thanks, please find my reply inline.
On 27/07/16 4:35 pm, "Marc-André Lureau" <marcandre.lur...@gmail.com> wrote: >Hi > >On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <saxenap....@gmail.com> wrote: >> From: Prerna Saxena <prerna.sax...@nutanix.com> >> >> 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 <prerna.sax...@nutanix.com> >> --- >> 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
