From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 9213B7D09 for ; Tue, 17 Apr 2018 11:03:23 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2018 02:03:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,463,1517904000"; d="scan'208";a="46791915" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.128]) ([10.237.220.128]) by fmsmga004.fm.intel.com with ESMTP; 17 Apr 2018 02:03:18 -0700 To: Yongseok Koh Cc: "dev@dpdk.org" , Adrien Mazarguil , =?UTF-8?Q?N=c3=a9lio_Laranjeiro?= , "keith.wiles@intel.com" , "jianfeng.tan@intel.com" , "andras.kovacs@ericsson.com" , "laszlo.vadkeri@ericsson.com" , "benjamin.walker@intel.com" , "bruce.richardson@intel.com" , Thomas Monjalon , "konstantin.ananyev@intel.com" , "kuralamudhan.ramakrishnan@intel.com" , "louise.m.daly@intel.com" , "pepperjo@japf.ch" , "jerin.jacob@caviumnetworks.com" , "hemant.agrawal@nxp.com" , "olivier.matz@6wind.com" , "shreyansh.jain@nxp.com" , "gowrishankar.m@linux.vnet.ibm.com" References: <0E4E13D3-8842-444E-BEF0-0D76B2547A57@mellanox.com> From: "Burakov, Anatoly" Message-ID: Date: Tue, 17 Apr 2018 10:03:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <0E4E13D3-8842-444E-BEF0-0D76B2547A57@mellanox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 44/70] net/mlx5: use virt2memseg instead of iteration X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Apr 2018 09:03:24 -0000 On 17-Apr-18 3:48 AM, Yongseok Koh wrote: > >> On Apr 11, 2018, at 5:30 AM, Anatoly Burakov wrote: >> >> Reduce dependency on internal details of EAL memory subsystem, and >> simplify code. >> >> Signed-off-by: Anatoly Burakov >> Tested-by: Santosh Shukla >> Tested-by: Hemant Agrawal >> Tested-by: Gowrishankar Muthukrishnan >> --- >> 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 > > -- Thanks, Anatoly