diff mbox

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

Message ID 0E4E13D3-8842-444E-BEF0-0D76B2547A57@mellanox.com (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply issues

Commit Message

Yongseok Koh April 17, 2018, 2:48 a.m. UTC 
  > On Apr 11, 2018, at 5:30 AM, Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
> Reduce dependency on internal details of EAL memory subsystem, and
> simplify code.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Tested-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Tested-by: Gowrishankar Muthukrishnan <gowrishankar.m@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.


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.


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

Comments

Burakov, Anatoly April 17, 2018, 9:03 a.m. UTC | #1 
On 17-Apr-18 3:48 AM, Yongseok Koh wrote:
> 
>> On Apr 11, 2018, at 5:30 AM, Anatoly Burakov <anatoly.burakov@intel.com> wrote:
>>
>> Reduce dependency on internal details of EAL memory subsystem, and
>> simplify code.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Tested-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> Tested-by: Gowrishankar Muthukrishnan <gowrishankar.m@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.

> 
> 
> 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
> 
>
Yongseok Koh April 17, 2018, 6:08 p.m. UTC | #2 
> On Apr 17, 2018, at 2:03 AM, Burakov, Anatoly <anatoly.burakov@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@intel.com> wrote:
>>> 
>>> Reduce dependency on internal details of EAL memory subsystem, and
>>> simplify code.
>>> 
>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>> Tested-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>>> Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> Tested-by: Gowrishankar Muthukrishnan <gowrishankar.m@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
diff mbox

Patch

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"