Re: [dpdk-dev] [PATCH 2/3] vhost: Fix RESET_OWNER handling not to close callfd

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

On Wed, Aug 19, 2015 at 06:51:08PM +0900, Tetsuya Mukawa wrote:
> When RESET_OWNER message is issued, vhost backend shouldn't close
> 'callfd', because it will be valid while vhost-user connection
> is established. It should be closed when connection is closed.

Doesn't it mean the end of connection when RESET_OWNER is received?

If you check my latest patch set for enabling vhost mq, you will
find free_device() will be invoked immediately once RESET_OWNER
signal is received.

    http://dpdk.org/ml/archives/dev/2015-September/023752.html

So, I doubt this patch is necessary.

BTW, does these 3 patches fix a real issue, or just some potential
issue in your mind? I saw that you mentioned your vhost pmd won't
work without those 3 patches, but I never saw you mentioned what
issue you fixed exactly. And it'd be good if you can post them in
commit log.

And, if we encounter same issue, will my above patch fix yours?

	--yliu
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_vhost/virtio-net.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 144f301..7794e8b 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -189,13 +189,9 @@ cleanup_device(struct virtio_net *dev)
>  		free(dev->mem);
>  	}
>  
> -	/* Close any event notifiers opened by device. */
> -	if ((int)dev->virtqueue[VIRTIO_RXQ]->callfd >= 0)
> -		close((int)dev->virtqueue[VIRTIO_RXQ]->callfd);
> +	/* Close kickfd event notifiers opened by device. */
>  	if ((int)dev->virtqueue[VIRTIO_RXQ]->kickfd >= 0)
>  		close((int)dev->virtqueue[VIRTIO_RXQ]->kickfd);
> -	if ((int)dev->virtqueue[VIRTIO_TXQ]->callfd >= 0)
> -		close((int)dev->virtqueue[VIRTIO_TXQ]->callfd);
>  	if ((int)dev->virtqueue[VIRTIO_TXQ]->kickfd >= 0)
>  		close((int)dev->virtqueue[VIRTIO_TXQ]->kickfd);
>  }
> @@ -206,6 +202,12 @@ cleanup_device(struct virtio_net *dev)
>  static void
>  free_device(struct virtio_net_config_ll *ll_dev)
>  {
> +	/* close callfd when connection is closed */
> +	if ((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd >= 0)
> +		close((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd);
> +	if ((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd >= 0)
> +		close((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd);
> +
>  	/* Free any malloc'd memory */
>  	rte_free(ll_dev->dev.virtqueue[VIRTIO_RXQ]);
>  	rte_free(ll_dev->dev.virtqueue[VIRTIO_TXQ]);
> @@ -241,6 +243,21 @@ rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
>  	}
>  }
>  
> +
> +/*
> + *  Rset variables in device structure.
> + */
> +static void
> +reset_device(struct virtio_net *dev)
> +{
> +	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
> +	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
> +
> +	/* Backends are set to -1 indicating an inactive device. */
> +	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
> +	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
> +}
> +
>  /*
>   *  Initialise all variables in device structure.
>   */
> @@ -261,14 +278,10 @@ init_device(struct virtio_net *dev)
>  	memset(dev->virtqueue[VIRTIO_RXQ], 0, sizeof(struct vhost_virtqueue));
>  	memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct vhost_virtqueue));
>  
> -	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
>  	dev->virtqueue[VIRTIO_RXQ]->callfd = (eventfd_t)-1;
> -	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
>  	dev->virtqueue[VIRTIO_TXQ]->callfd = (eventfd_t)-1;
>  
> -	/* Backends are set to -1 indicating an inactive device. */
> -	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
> -	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
> +	reset_device(dev);
>  }
>  
>  /*
> @@ -403,7 +416,7 @@ reset_owner(struct vhost_device_ctx ctx)
>  	ll_dev = get_config_ll_entry(ctx);
>  
>  	cleanup_device(&ll_dev->dev);
> -	init_device(&ll_dev->dev);
> +	reset_device(&ll_dev->dev);
>  
>  	return 0;
>  }
> -- 
> 2.1.4