> On Apr 11, 2018, at 5:30 AM, Anatoly Burakov <anatoly.bura...@intel.com> 
> wrote:
> 
> Reduce dependency on internal details of EAL memory subsystem, and
> simplify code.
> 
> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
> Tested-by: Santosh Shukla <santosh.shu...@caviumnetworks.com>
> Tested-by: Hemant Agrawal <hemant.agra...@nxp.com>
> Tested-by: Gowrishankar Muthukrishnan <gowrishanka...@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.


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

Reply via email to