The loop to unwind existing mmaps was only unmapping the first segment. Also, remove obvious redundant assignment. Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists") Cc: anatoly.burakov@intel.com Cc: stable@dpdk.org Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- lib/librte_eal/linux/eal/eal_memory.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c index 43e4ffc757bd..cf5b2433614b 100644 --- a/lib/librte_eal/linux/eal/eal_memory.c +++ b/lib/librte_eal/linux/eal/eal_memory.c @@ -1967,9 +1967,8 @@ eal_legacy_hugepage_attach(void) close(fd); error: /* map all segments into memory to make sure we get the addrs */ - cur_seg = 0; for (cur_seg = 0; cur_seg < i; cur_seg++) { - struct hugepage_file *hf = &hp[i]; + struct hugepage_file *hf = &hp[cur_seg]; size_t map_sz = hf->size; void *map_addr = hf->final_va; -- 2.20.1
On Mon, Jan 6, 2020 at 9:56 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > The loop to unwind existing mmaps was only unmapping the > first segment. > > Also, remove obvious redundant assignment. > > Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists") > Cc: anatoly.burakov@intel.com > Cc: stable@dpdk.org > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > lib/librte_eal/linux/eal/eal_memory.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c > index 43e4ffc757bd..cf5b2433614b 100644 > --- a/lib/librte_eal/linux/eal/eal_memory.c > +++ b/lib/librte_eal/linux/eal/eal_memory.c > @@ -1967,9 +1967,8 @@ eal_legacy_hugepage_attach(void) > close(fd); > error: > /* map all segments into memory to make sure we get the addrs */ This looks like a copy of the same loop from a few lines before in the same function. The comment can be removed. > - cur_seg = 0; > for (cur_seg = 0; cur_seg < i; cur_seg++) { > - struct hugepage_file *hf = &hp[i]; > + struct hugepage_file *hf = &hp[cur_seg]; > size_t map_sz = hf->size; > void *map_addr = hf->final_va; > map_addr and map_sz are unnecessary. We should be safe with dereferencing hp if i != 0, but still how about: error: if (hp != NULL && hp != MAP_FAILED) { unsigned int cur_seg; for (cur_seg = 0; cur_seg < i; cur_seg++) munmap(hp[cur_seg].final_va, hp[cur_seg].size); munmap(hp, size); } if (fd_hugepage >= 0) close(fd_hugepage); return -1; -- David Marchand
On 06-Jan-20 8:55 PM, Stephen Hemminger wrote:
> The loop to unwind existing mmaps was only unmapping the
> first segment.
>
> Also, remove obvious redundant assignment.
>
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
--
Thanks,
Anatoly
The loop to unwind existing mmaps was only unmapping the first segment and the error paths after mmap() were not doing munmap of the current segment. Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists") Cc: anatoly.burakov@intel.com Cc: stable@dpdk.org Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com> --- v2 - incorporate feedback from David Marchand fix missing munmap of current segment lib/librte_eal/linux/eal/eal_memory.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c index 43e4ffc757bd..5604c2a7c04d 100644 --- a/lib/librte_eal/linux/eal/eal_memory.c +++ b/lib/librte_eal/linux/eal/eal_memory.c @@ -1928,7 +1928,7 @@ eal_legacy_hugepage_attach(void) if (flock(fd, LOCK_SH) < 0) { RTE_LOG(DEBUG, EAL, "%s(): Locking file failed: %s\n", __func__, strerror(errno)); - goto fd_error; + goto mmap_error; } /* find segment data */ @@ -1936,13 +1936,13 @@ eal_legacy_hugepage_attach(void) if (msl == NULL) { RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg list\n", __func__); - goto fd_error; + goto mmap_error; } ms = rte_mem_virt2memseg(map_addr, msl); if (ms == NULL) { RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg\n", __func__); - goto fd_error; + goto mmap_error; } msl_idx = msl - mcfg->memsegs; @@ -1950,7 +1950,7 @@ eal_legacy_hugepage_attach(void) if (ms_idx < 0) { RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg idx\n", __func__); - goto fd_error; + goto mmap_error; } /* store segment fd internally */ @@ -1963,18 +1963,15 @@ eal_legacy_hugepage_attach(void) close(fd_hugepage); return 0; +mmap_error: + munmap(hp[i].final_va, hp[i].size); fd_error: close(fd); error: - /* map all segments into memory to make sure we get the addrs */ - cur_seg = 0; - for (cur_seg = 0; cur_seg < i; cur_seg++) { - struct hugepage_file *hf = &hp[i]; - size_t map_sz = hf->size; - void *map_addr = hf->final_va; + /* unwind mmap's done so far */ + for (cur_seg = 0; cur_seg < i; cur_seg++) + munmap(hp[cur_seg].final_va, hp[cur_seg].size); - munmap(map_addr, map_sz); - } if (hp != NULL && hp != MAP_FAILED) munmap(hp, size); if (fd_hugepage >= 0) -- 2.20.1
22/01/2020 18:06, Stephen Hemminger:
> The loop to unwind existing mmaps was only unmapping the
> first segment and the error paths after mmap() were not
> doing munmap of the current segment.
>
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> v2 - incorporate feedback from David Marchand
> fix missing munmap of current segment
Applied, thanks