[dpdk-dev] [PATCH v6 44/70] net/mlx5: use virt2memseg instead of iteration

[Yongseok Koh]

> On Apr 17, 2018, at 2:03 AM, Burakov, Anatoly <anatoly.burakov at intel.com> wrote:
> 
> On 17-Apr-18 3:48 AM, Yongseok Koh wrote:
>>> On Apr 11, 2018, at 5:30 AM, Anatoly Burakov <anatoly.burakov at intel.com> wrote:
>>> 
>>> Reduce dependency on internal details of EAL memory subsystem, and
>>> simplify code.
>>> 
>>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>> Tested-by: Santosh Shukla <santosh.shukla at caviumnetworks.com>
>>> Tested-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>>> Tested-by: Gowrishankar Muthukrishnan <gowrishankar.m at linux.vnet.ibm.com>
>>> ---
>>> drivers/net/mlx5/mlx5_mr.c | 19 ++++++++-----------
>>> 1 file changed, 8 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c
>>> index 58afeb7..c96e134 100644
>>> --- a/drivers/net/mlx5/mlx5_mr.c
>>> +++ b/drivers/net/mlx5/mlx5_mr.c
>>> @@ -234,10 +234,9 @@ struct mlx5_mr *
>>> mlx5_mr_new(struct rte_eth_dev *dev, struct rte_mempool *mp)
>>> {
>>> 	struct priv *priv = dev->data->dev_private;
>>> -	const struct rte_memseg *ms = rte_eal_get_physmem_layout();
>>> +	const struct rte_memseg *ms;
>>> 	uintptr_t start;
>>> 	uintptr_t end;
>>> -	unsigned int i;
>>> 	struct mlx5_mr *mr;
>>> 
>>> 	mr = rte_zmalloc_socket(__func__, sizeof(*mr), 0, mp->socket_id);
>>> @@ -261,17 +260,15 @@ mlx5_mr_new(struct rte_eth_dev *dev, struct rte_mempool *mp)
>>> 	/* Save original addresses for exact MR lookup. */
>>> 	mr->start = start;
>>> 	mr->end = end;
>>> +
>>> 	/* Round start and end to page boundary if found in memory segments. */
>>> -	for (i = 0; (i < RTE_MAX_MEMSEG) && (ms[i].addr != NULL); ++i) {
>>> -		uintptr_t addr = (uintptr_t)ms[i].addr;
>>> -		size_t len = ms[i].len;
>>> -		unsigned int align = ms[i].hugepage_sz;
>>> +	ms = rte_mem_virt2memseg((void *)start);
>>> +	if (ms != NULL)
>>> +		start = RTE_ALIGN_FLOOR(start, ms->hugepage_sz);
>>> +	ms = rte_mem_virt2memseg((void *)end);
>>> +	if (ms != NULL)
>>> +		end = RTE_ALIGN_CEIL(end, ms->hugepage_sz);
>> It is buggy. The memory region is [start, end), so if the memseg of 'end' isn't
>> allocated yet, the returned ms will have zero entries and this will make 'end'
>> zero. Instead, the following will be fine.
>> diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c
>> index fdf7b3e88..39bbe2481 100644
>> --- a/drivers/net/mlx5/mlx5_mr.c
>> +++ b/drivers/net/mlx5/mlx5_mr.c
>> @@ -265,9 +265,7 @@ mlx5_mr_new(struct rte_eth_dev *dev, struct rte_mempool *mp)
>>         ms = rte_mem_virt2memseg((void *)start, NULL);
>>         if (ms != NULL)
>>                 start = RTE_ALIGN_FLOOR(start, ms->hugepage_sz);
>> -       ms = rte_mem_virt2memseg((void *)end, NULL);
>> -       if (ms != NULL)
>> -               end = RTE_ALIGN_CEIL(end, ms->hugepage_sz);
>> +       end = RTE_ALIGN_CEIL(end, ms->hugepage_sz);
>>         DRV_LOG(DEBUG,
>>                 "port %u mempool %p using start=%p end=%p size=%zu for memory"
>> Same for mlx4. Please fix both mlx5 and mlx4 so that we can verify the new design.
>> However, this code block will be removed eventually. I've done a patchset to
>> accommodate your memory hotplug design and I'll send it out soon.
> 
> Hi,
> 
> Thanks for raising this. I'll submit a patch shortly.

I didn't notice that your patchset has been merged. I thought you were to send out
a new version.

Never mind. I'll send out a fix.

Thanks,
Yongseok


> 
>> Thanks in advance.
>> Yongseok
>>> -		if ((start > addr) && (start < addr + len))
>>> -			start = RTE_ALIGN_FLOOR(start, align);
>>> -		if ((end > addr) && (end < addr + len))
>>> -			end = RTE_ALIGN_CEIL(end, align);
>>> -	}
>>> 	DRV_LOG(DEBUG,
>>> 		"port %u mempool %p using start=%p end=%p size=%zu for memory"
>>> 		" region",
>>> -- 
>>> 2.7.4
> 
> 
> -- 
> Thanks,
> Anatoly