Hi Marc, Thank you for taking a look.
On 25/07/16 3:57 pm, "Marc-André Lureau" <marcandre.lur...@gmail.com> wrote: >Hi > >On Mon, Jul 25, 2016 at 10:41 AM, Prerna <saxenap....@gmail.com> wrote: >> >> >> On Thu, Jul 7, 2016 at 12:04 PM, Prerna Saxena <saxenap....@gmail.com> >> wrote: >>> >>> From: Prerna Saxena <prerna.sax...@nutanix.com> >>> >>> 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
