Re: [dpdk-dev] [PATCH] vhost: Fix reset_owner message handling not to clear callfd

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Thu, Nov 19, 2015 at 03:23:26PM +0900, Tetsuya Mukawa wrote:
> The patch fixes reset_owner message handling not to clear callfd,
> because callfd will be valid while connection is establihed.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_vhost/virtio-net.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 886c104..ae1e4bd 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -322,6 +322,25 @@ init_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
>  	init_vring_queue(dev->virtqueue[base_idx + VIRTIO_TXQ], qp_idx);
>  }
>  
> +static void
> +reset_vring_queue(struct vhost_virtqueue *vq, int qp_idx)
> +{
> +	int callfd;
> +
> +	callfd = vq->callfd;
> +	init_vring_queue(vq, qp_idx);
> +	vq->callfd = callfd;

It may fix your issue, but it's not a proper fix. As we actually closed
callfd at cleanup_vq().

BTW, the name, cleanup_vq/device, is not well taken. For a name like
cleanup, those functions should be invoked at device remove stage (say,
shutting down a VM), but not at reset stage (which is actually treated
as STOP in current QEMU implementation).

I have a plan to correct it, but I guess it's not the right time as
v2.2 is coming soon. However, for a fix, it could be simple: how about
adding another arg to cleanup_vq:

	cleanup_vq(struct vhost_virtqueue *vq, int destory)

And we only close callfd when destroy flag is set. (And of course, this
patch is needed).

What do you think of that?

	--yliu
> +}
> +
> +static void
> +reset_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
> +{
> +	uint32_t base_idx = qp_idx * VIRTIO_QNUM;
> +
> +	reset_vring_queue(dev->virtqueue[base_idx + VIRTIO_RXQ], qp_idx);
> +	reset_vring_queue(dev->virtqueue[base_idx + VIRTIO_TXQ], qp_idx);
> +}
> +
>  static int
>  alloc_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
>  {
> @@ -362,7 +381,7 @@ reset_device(struct virtio_net *dev)
>  	dev->flags = 0;
>  
>  	for (i = 0; i < dev->virt_qp_nb; i++)
> -		init_vring_queue_pair(dev, i);
> +		reset_vring_queue_pair(dev, i);
>  }
>  
>  /*
> -- 
> 2.1.4