[dpdk-stable] patch 'vhost: fix device cleanup at stop' has been queued to LTS release 16.11.7

[Luca Boccassi]

On Wed, 2018-05-23 at 09:35 +0200, Maxime Coquelin wrote:
> Hi Luca, Tomasz,
> 
> While testing 16.11 branch, I noticed vhost lib is broken.
> The symptoms is no packets are sent or received.
> 
> I ran a bisect which points to this commit. Reverting it solves the
> issue.
> 
> Debugging a bit more, I can see that the callfd is valid when no
> packets
> are being transmitted. I think the problem is that the callfd is
> received after the kickfd:
> 
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> VHOST_CONFIG: vring kick idx:0 file:16
> VHOST_CONFIG: virtio is not ready for processing.
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:0 file:17
> 
> And in 16.11, the new_device callback is called from the
> VHOST_USER_SET_VRING_KICK handling, only if the device is ready.
> This is different in later versions, where the new_device callback
> can be called for any request.
> 
> The right way to fix this would be to move the new_device callback
> call
> for any request, but I think there is a non-negligible risk of
> regression so we'd need to be careful doing that.
> 
> Other option is to simply revert Tomasz patch in 16.11 LTS. This is
> not
> ideal because Tomasz patch is fixing a real issue (new_device get
> called
> with an outdated/invalid callfd). However, I don't know if it has
> real
> consequences, as the callfd is updated right after.
> 
> Except if someone face real issue due to callfd being updated after
> new_device is called, then my suggestion would be to revert Tomasz
> patch
> as we are at 6 months of 16.11 EOL.
> 
> Luca, Tomasz, what's your take on this?
> 
> Regards,
> Maxime

I'm fine with your suggestion of reverting the patch, as it seems less
risky.

> On 05/01/2018 12:44 PM, luca.boccassi at gmail.com wrote:
> > Hi,
> > 
> > FYI, your patch has been queued to LTS release 16.11.7
> > 
> > Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable
> > yet.
> > It will be pushed if I get no objections before 05/03/18. So please
> > shout if anyone has objections.
> > 
> > Thanks.
> > 
> > Luca Boccassi
> > 
> > ---
> >  From 357f27736c79d0258409666277b6e113d11c757b Mon Sep 17 00:00:00
> > 2001
> > From: Tomasz Kulasek <tomaszx.kulasek at intel.com>
> > Date: Fri, 9 Feb 2018 18:10:00 +0100
> > Subject: [PATCH] vhost: fix device cleanup at stop
> > 
> > [ upstream commit ace7b6b7859e1dc410589610a8e436c1a3b430f3 ]
> > 
> > This prevents from destroying & recreating user device in
> > "incomplete"
> > vring state. virtio_is_ready() was returning true for devices with
> > vrings which did not have valid callfd (their
> > VHOST_USER_SET_VRING_CALL
> > hasn't arrived yet)
> > 
> > Fixes: 8f972312b8f4 ("vhost: support vhost-user")
> > 
> > Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk at intel.com>
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek at intel.com>
> > Reviewed-by: Jianfeng Tan <jianfeng.tan at intel.com>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin at redhat.com>
> > ---
> >   lib/librte_vhost/vhost_user.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/lib/librte_vhost/vhost_user.c
> > b/lib/librte_vhost/vhost_user.c
> > index abcb6ac14..0d30f11ef 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -800,6 +800,11 @@ vhost_user_get_vring_base(struct virtio_net
> > *dev,
> >   
> >   	vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
> >   
> > +	if (vq->callfd >= 0)
> > +		close(vq->callfd);
> > +
> > +	vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD;
> > +
> >   	if (dev->dequeue_zero_copy)
> >   		free_zmbufs(vq);
> >   	rte_free(vq->shadow_used_ring);
> > 

-- 
Kind regards,
Luca Boccassi