Re: [dpdk-dev] [PATCH v2] eal/linux: do not create user mem map repeatedly when it exists

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 30-Jul-20 2:16 PM, wangyunjian wrote:
>> -----Original Message-----
>> From: Burakov, Anatoly [mailto:anatoly.burakov@intel.com]
>> Sent: Monday, July 27, 2020 5:24 PM
>> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org;
>> david.marchand@redhat.com
>> Cc: Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
>> <xudingke@huawei.com>; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] eal/linux: do not create user mem map
>> repeatedly when it exists
>>
>> On 25-Jul-20 10:59 AM, wangyunjian wrote:
>>>> -----Original Message-----
>>>> From: Burakov, Anatoly [mailto:anatoly.burakov@intel.com]
>>>> Sent: Friday, July 24, 2020 9:25 PM
>>>> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org;
>>>> david.marchand@redhat.com
>>>> Cc: Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
>>>> <xudingke@huawei.com>; stable@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2] eal/linux: do not create user mem
>>>> map repeatedly when it exists
>>>>
>>>> On 23-Jul-20 3:48 PM, wangyunjian wrote:
>>>>> From: Yunjian Wang <wangyunjian@huawei.com>
>>>>>
>>>>> Currently, we will create new user mem map entry for the same memory
>>>>> segment, but in fact it has already been added to the user mem maps.
>>>>> It's not necessary to create it twice.
>>>>>
>>>>> To resolve the issue, add support to remove the same entry in the
>>>>> function compact_user_maps().
>>>>>
>>>>> Fixes: 0cbce3a167f1 ("vfio: skip DMA map failure if already mapped")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>>>> ---
>>>>> v2:
>>>>> * Remove the same entry in the function compact_user_maps()
>>>>> ---
>>>>>     lib/librte_eal/linux/eal_vfio.c | 5 +++++
>>>>>     1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_eal/linux/eal_vfio.c
>>>>> b/lib/librte_eal/linux/eal_vfio.c index abb12a354..df99307b7 100644
>>>>> --- a/lib/librte_eal/linux/eal_vfio.c
>>>>> +++ b/lib/librte_eal/linux/eal_vfio.c
>>>>> @@ -167,6 +167,10 @@ adjust_map(struct user_mem_map *src, struct
>>>> user_mem_map *end,
>>>>>     static int
>>>>>     merge_map(struct user_mem_map *left, struct user_mem_map
>> *right)
>>>>>     {
>>>>> +	/* merge the same maps into one */
>>>>> +	if (memcmp(left, right, sizeof(struct user_mem_map)) == 0)
>>>>> +		goto out;
>>>>> +
>>>>
>>>> merge_map looks for adjacent maps only, but does not handle maps that
>>>> are wholly contained within one another ("the same map" also matches
>>>> this definition). wouldn't it be better to check for that instead of
>>>> *just* handling identical maps?
>>>
>>> What about using the initial implementation?
>>> We don't create new user mem map entry for the same memory segment.
>>
>> I don't like this implementation because it relies on particulars of how VFIO
>> mapping work without explicitly specifying them. I.e. it's prone to breaking
>> when changing code. That's not even mentioning that we have no guarantees
>> on kernel behavior in that particular case being identical on all supported
>> platforms.
>>
>> I would honestly prefer an explicit compaction over implicit one.
> 
> What about this implementation?

Again, this works, but i feel like specializing it to just merge the 
exact same maps is missing an opportunity to provide a more general 
solution that merges same *and* subset maps.

> 
> diff --git a/lib/librte_eal/linux/eal_vfio.c b/lib/librte_eal/linux/eal_vfio.c
> index e07979936..8dcb04cd9 100644
> --- a/lib/librte_eal/linux/eal_vfio.c
> +++ b/lib/librte_eal/linux/eal_vfio.c
> @@ -179,6 +179,19 @@ merge_map(struct user_mem_map *left, struct user_mem_map *right)
>   	return 1;
>   }
>   
> +/* try merging two same maps into one, return 1 if succeeded */
> +static int
> +merge_same_map(struct user_mem_map *left, struct user_mem_map *right)
> +{
> +	if (memcmp(left, right, sizeof(struct user_mem_map)) != 0) {
> +		return 0;
> +	}
> +
> +	memset(right, 0, sizeof(*right));
> +
> +	return 1;
> +}
> +
>   static struct user_mem_map *
>   find_user_mem_map(struct user_mem_maps *user_mem_maps, uint64_t addr,
>   		uint64_t iova, uint64_t len)
> @@ -232,7 +245,7 @@ compact_user_maps(struct user_mem_maps *user_mem_maps)
>   		if (is_null_map(l) || is_null_map(r))
>   			continue;
>   
> -		if (merge_map(l, r))
> +		if (merge_map(l, r) || merge_same_map(l, r))
>   			n_merged++;
>   	}
> 
> Thanks,
> Yunjian
> 
>>
>>>
>>> @@ -1828,6 +1828,13 @@  container_dma_map(struct vfio_config
>> *vfio_cfg, uint64_t vaddr, uint64_t iova,
>>>    		ret = -1;
>>>    		goto out;
>>>    	}
>>> +
>>> +	/* we don't need create new user mem map entry
>>> +	 * for the same memory segment.
>>> +	 */
>>> +	if (errno == EBUSY || errno == EEXIST)
>>> +		goto out;
>>> +
>>>    	/* create new user mem map entry */
>>>    	new_map =
>> &user_mem_maps->maps[user_mem_maps->n_maps++];
>>>    	new_map->addr = vaddr;
>>>
>>> Thanks,
>>> Yunjian
>>>>
>>>>>     	if (left->addr + left->len != right->addr)
>>>>>     		return 0;
>>>>>     	if (left->iova + left->len != right->iova) @@ -174,6 +178,7 @@
>>>>> merge_map(struct user_mem_map *left, struct
>>>> user_mem_map *right)
>>>>>
>>>>>     	left->len += right->len;
>>>>>
>>>>> +out:
>>>>>     	memset(right, 0, sizeof(*right));
>>>>>
>>>>>     	return 1;
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Anatoly
>>
>>
>> --
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly