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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 10/05/2018 02:34 PM, Ilya Maximets wrote:
> 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().

Right, I agree this is vhost_backend_cleanup()'s job. I will remove it
from here.

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