Re: [dpdk-dev] [PATCH v3 13/19] vhost: register new regions with userfaultfd

[Ilya Maximets <i.maximets@samsung.com>]

On 04.10.2018 11:13, Maxime Coquelin wrote:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index ffb3ea60a..d61be2da6 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -968,6 +968,32 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  			mmap_size,
>  			alignment,
>  			mmap_offset);
> +
> +		if (dev->postcopy_listening) {
> +#ifdef RTE_LIBRTE_VHOST_POSTCOPY
> +			struct uffdio_register reg_struct;
> +
> +			reg_struct.range.start = (uint64_t)(uintptr_t)mmap_addr;
> +			reg_struct.range.len = mmap_size;
> +			reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
> +
> +			if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER,
> +						&reg_struct)) {
> +				RTE_LOG(ERR, VHOST_CONFIG,
> +						"Failed to register ufd for region %d: (ufd = %d) %s\n",
> +						i, dev->postcopy_ufd,
> +						strerror(errno));
> +				goto err_ufd;
> +			}
> +			RTE_LOG(INFO, VHOST_CONFIG,
> +					"\t userfaultfd registered for range : %llx - %llx\n",
> +					reg_struct.range.start,
> +					reg_struct.range.start +
> +					reg_struct.range.len - 1);
> +#else
> +			goto err_ufd;
> +#endif
> +		}
>  	}
>  
>  	for (i = 0; i < dev->nr_vring; i++) {
> @@ -983,7 +1009,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  
>  			dev = translate_ring_addresses(dev, i);
>  			if (!dev)
> -				goto err_mmap;
> +				goto err_ufd;
>  
>  
>  			*pdev = dev;
> @@ -994,6 +1020,11 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  
>  	return VH_RESULT_OK;
>  
> +err_ufd:
> +	if (dev->postcopy_ufd >= 0) {
> +		close(dev->postcopy_ufd);
> +		dev->postcopy_ufd = -1;
> +	}

It's unclear why we need to close postcopy_ufd here because this
handler doesn't allocate it and postcopy_ufd should exist before
calling this function.
So, IMHO, we need to close it in all the VH_RESULT_ERR cases or
leave it for later vhost_backend_cleanup().

>  err_mmap:
>  	free_mem_region(dev);
>  	rte_free(dev->mem);
>