From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40713) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSOtP-0003GJ-FN for qemu-devel@nongnu.org; Wed, 27 Jul 2016 09:29:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSOtK-0007N0-GA for qemu-devel@nongnu.org; Wed, 27 Jul 2016 09:28:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59484) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSOtK-0007Mq-7a for qemu-devel@nongnu.org; Wed, 27 Jul 2016 09:28:54 -0400 Date: Wed, 27 Jul 2016 16:28:49 +0300 From: "Michael S. Tsirkin" Message-ID: <20160727132849.GC9717@redhat.com> References: <1469613157-8942-1-git-send-email-saxenap.ltc@gmail.com> <1469613157-8942-2-git-send-email-saxenap.ltc@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Prerna Saxena Cc: =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , Prerna Saxena , QEMU , Felipe Franciosi , Anil Kumar Boggarapu On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote: > Hi Marc, > Thanks, please find my reply inline. >=20 >=20 >=20 >=20 >=20 > On 27/07/16 4:35 pm, "Marc-Andr=C3=A9 Lureau" wrote: >=20 > >Hi > > > >On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena = wrote: > >> From: Prerna Saxena > >> > >> 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 > >> --- > >> 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_RE= PLY_ACK for > >> + details. > > > >Why need_response and not "need reply"? >=20 > (I=E2=80=99d already pointed this out earlier, but looks like I was pos= sibly not very clear.) > Before deciding on the right name for Bit 3, let us see the nomenclatur= e for Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each r= eply from the slave=E2=80=9D. > So we already have a _reply_ flag in use. If the name Bit 3 as the _nee= d_reply_ flag, don=E2=80=99t you think it would be ultra-confusing ? I fo= und it confusing when I reviewed the documentation with this different t= erm. > So I chose the name need_response with much deliberation =E2=80=94 it c= onveys 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=C3=A9. > > > >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 descript= ors 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 t= he guest to > >> allow the vhost user backend to construct and broadcast the f= ake RARP. > >> + > >> +VHOST_USER_PROTOCOL_F_REPLY_ACK: > >> +------------------------------- > >> +The original vhost-user specification only demands responses for ce= rtain > > > >responses/replies >=20 > If you feel strongly about it, will change it here. >=20 > > > >> +commands. This differs from the vhost protocol implementation where= commands > >> +are sent over an ioctl() call and block until the client has comple= ted. > >> + > >> +With this protocol extension negotiated, the sender (QEMU) can set = the newly > >> +introduced "need_response" [Bit 3] flag to any command. This indica= tes that > > > >need reply, you can remove the "newly introduced" (it's not going to > >be so new after a while) >=20 > * need_reply =3D no I don=E2=80=99t agree, for reasons cited earlier. > * remove the =E2=80=9Cnewly introduced=E2=80=9D phrase =3D agree, will = do. >=20 > > > >> +the client MUST respond with a Payload VhostUserMsg indicating succ= ess 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. >=20 > I don=E2=80=99t currently have any code that raises an error for such c= ases. > The implementation silently ignores it. >=20 > > > >> +failure. The payload should be set to zero on success or non-zero o= n 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 fail= ure. > >> + > >> +This indicates to QEMU that the requested operation has determinist= ically > >> +been met or not. Today, QEMU is expected to terminate the main vhos= t-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 fo= ur messages > >> +anyway require distinct responses from the vhost-user client proces= s: > > > >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) >=20 > Ok, will remove it for brevity. >=20 > > > >> + * 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_REPL= Y_ACK or > >> +need_response bit being set brings no behaviourial change. > > > >Reply >=20 > Again, I disagree, for reasons cited above. >=20 > [..snip..] removing the rest. > > > > > > > >--=20 > >Marc-Andr=C3=A9 Lureau >=20 > Thanks once again for the quick review. > Let me know if this makes sense, so I=E2=80=99ll quickly spin a cleaned= -up patch which includes the tiny documentation changes. >=20 > Regards, > Prerna