On Fri, Jan 13, 2017 at 03:09:46PM +0000, Felipe Franciosi wrote:
> Hi Marc-Andre,
>
> > On 13 Jan 2017, at 07:03, Marc-André Lureau <mlur...@redhat.com> wrote:
> >
> > Hi
> >
> > ----- Original Message -----
> >> Currently, VQs are started as soon as a SET_VRING_KICK is received. That
> >> is too early in the VQ setup process, as the backend might not yet have
> >
> > I think we may want to reconsider queue_set_started(), move it elsewhere,
> > since kick/call fds aren't mandatory to process the rings.
>
> Hmm. The fds aren't mandatory, but I imagine in that case we should still
> receive SET_VRING_KICK/CALL messages without an fd (ie. with the
> VHOST_MSG_VQ_NOFD_MASK flag set). Wouldn't that be the case?
Please look at docs/specs/vhost-user.txt, Starting and stopping rings
The spec says:
Client must start ring upon receiving a kick (that is, detecting that
file descriptor is readable) on the descriptor specified by
VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
VHOST_USER_GET_VRING_BASE.
> >
> >> a callfd to notify in case it received a kick and fully processed the
> >> request/command. This patch only starts a VQ when a SET_VRING_CALL is
> >> received.
> >
> > I don't like that much, as soon as the kick fd is received, it should start
> > polling it imho. callfd is optional, it may have one and not the other.
>
> So the question is whether we should be receiving a SET_VRING_CALL anyway or
> not, regardless of an fd being sent. (I think we do, but I haven't done
> extensive testing with other device types.)
I would say not, only KICK is mandatory and that is also not enough
to process ring. You must wait for it to be readable.
> >
> > Perhaps it's best for now to delay the callfd notification with a flag
> > until it is received?
>
> The other idea is to always kick when we receive the callfd. I remember
> discussing that alternative with you before libvhost-user went in. The
> protocol says both the driver and the backend must handle spurious kicks.
> This approach also fixes the bug.
>
> I'm happy with whatever alternative you want, as long it makes libvhost-user
> usable for storage devices.
>
> Thanks,
> Felipe
>
>
> >
> >
> >> Signed-off-by: Felipe Franciosi <fel...@nutanix.com>
> >> ---
> >> contrib/libvhost-user/libvhost-user.c | 26 +++++++++++++-------------
> >> 1 file changed, 13 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/contrib/libvhost-user/libvhost-user.c
> >> b/contrib/libvhost-user/libvhost-user.c
> >> index af4faad..a46ef90 100644
> >> --- a/contrib/libvhost-user/libvhost-user.c
> >> +++ b/contrib/libvhost-user/libvhost-user.c
> >> @@ -607,19 +607,6 @@ vu_set_vring_kick_exec(VuDev *dev, VhostUserMsg *vmsg)
> >> DPRINT("Got kick_fd: %d for vq: %d\n", vmsg->fds[0], index);
> >> }
> >>
> >> - dev->vq[index].started = true;
> >> - if (dev->iface->queue_set_started) {
> >> - dev->iface->queue_set_started(dev, index, true);
> >> - }
> >> -
> >> - if (dev->vq[index].kick_fd != -1 && dev->vq[index].handler) {
> >> - dev->set_watch(dev, dev->vq[index].kick_fd, VU_WATCH_IN,
> >> - vu_kick_cb, (void *)(long)index);
> >> -
> >> - DPRINT("Waiting for kicks on fd: %d for vq: %d\n",
> >> - dev->vq[index].kick_fd, index);
> >> - }
> >> -
> >> return false;
> >> }
> >>
> >> @@ -661,6 +648,19 @@ vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
> >>
> >> DPRINT("Got call_fd: %d for vq: %d\n", vmsg->fds[0], index);
> >>
> >> + dev->vq[index].started = true;
> >> + if (dev->iface->queue_set_started) {
> >> + dev->iface->queue_set_started(dev, index, true);
> >> + }
> >> +
> >> + if (dev->vq[index].kick_fd != -1 && dev->vq[index].handler) {
> >> + dev->set_watch(dev, dev->vq[index].kick_fd, VU_WATCH_IN,
> >> + vu_kick_cb, (void *)(long)index);
> >> +
> >> + DPRINT("Waiting for kicks on fd: %d for vq: %d\n",
> >> + dev->vq[index].kick_fd, index);
> >> + }
> >> +
> >> return false;
> >> }
> >>
> >> --
> >> 1.9.4
> >>
> >>
>
