| Message ID | 1421938855-6814-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 01/22/2015 06:18 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov >> Sent: Thursday, January 22, 2015 5:01 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH] linux-generic: shm fix unmap for hugepages >> >> In case of hugepages unmap has to be done with address aligned to >> page size. Also unmap() has to be done for address returned by mmap(), >> not for aligned address after that. >> >> To reproduce original bug run: >> echo 4096 > /proc/sys/vm/nr_hugepages >> make check >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> platform/linux-generic/odp_shared_memory.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux- >> generic/odp_shared_memory.c >> index 99c5b40..51eba02 100644 >> --- a/platform/linux-generic/odp_shared_memory.c >> +++ b/platform/linux-generic/odp_shared_memory.c >> @@ -134,8 +134,14 @@ int odp_shm_free(odp_shm_t shm) >> odp_spinlock_lock(&odp_shm_tbl->lock); >> shm_block = &odp_shm_tbl->block[i]; >> >> +#ifdef MAP_HUGETLB >> + /* round up alloc size by page */ >> + alloc_size = (shm_block->size + (shm_block->page_sz - 1)) >> + & (-shm_block->page_sz); > Could you rebase this on top of my two patches. And then move this calculation to shm_reserve() side, so that the same size gets allocated, saved into block->alloc_size, and the freed here. > > -Petri Ok, I saw your patches after I wrote mine. Will rebase it on top. Maxim. > >> +#else >> alloc_size = shm_block->size + shm_block->align; >> - ret = munmap(shm_block->addr, alloc_size); >> +#endif >> + ret = munmap(shm_block->addr_orig, alloc_size); >> if (0 != ret) { >> ODP_DBG("odp_shm_free: munmap failed\n"); >> odp_spinlock_unlock(&odp_shm_tbl->lock); >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
Can we add a validation case for this bug, Maxim can that be part of this patch set, looks fairly simple to add as a second patch A very quick google suggests that a test case that calls "hugeadm --explain" or one of the many other options and succeeds might be a good way for the test case to know if it should run. Possibly better is doing that test in automake so that it can set a flag for hugepage which will then modify the list of test cases run. My quick google did not find a more portable way to know if HP was available. Mike On 22 January 2015 at 10:25, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 01/22/2015 06:18 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> -----Original Message----- >>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >>> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov >>> Sent: Thursday, January 22, 2015 5:01 PM >>> To: lng-odp@lists.linaro.org >>> Subject: [lng-odp] [PATCH] linux-generic: shm fix unmap for hugepages >>> >>> In case of hugepages unmap has to be done with address aligned to >>> page size. Also unmap() has to be done for address returned by mmap(), >>> not for aligned address after that. >>> >>> To reproduce original bug run: >>> echo 4096 > /proc/sys/vm/nr_hugepages >>> make check >>> >>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>> --- >>> platform/linux-generic/odp_shared_memory.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_shared_memory.c >>> b/platform/linux- >>> generic/odp_shared_memory.c >>> index 99c5b40..51eba02 100644 >>> --- a/platform/linux-generic/odp_shared_memory.c >>> +++ b/platform/linux-generic/odp_shared_memory.c >>> @@ -134,8 +134,14 @@ int odp_shm_free(odp_shm_t shm) >>> odp_spinlock_lock(&odp_shm_tbl->lock); >>> shm_block = &odp_shm_tbl->block[i]; >>> >>> +#ifdef MAP_HUGETLB >>> + /* round up alloc size by page */ >>> + alloc_size = (shm_block->size + (shm_block->page_sz - 1)) >>> + & (-shm_block->page_sz); >>> >> Could you rebase this on top of my two patches. And then move this >> calculation to shm_reserve() side, so that the same size gets allocated, >> saved into block->alloc_size, and the freed here. >> >> -Petri >> > > Ok, I saw your patches after I wrote mine. Will rebase it on top. > > Maxim. > > > >> +#else >>> alloc_size = shm_block->size + shm_block->align; >>> - ret = munmap(shm_block->addr, alloc_size); >>> +#endif >>> + ret = munmap(shm_block->addr_orig, alloc_size); >>> if (0 != ret) { >>> ODP_DBG("odp_shm_free: munmap failed\n"); >>> odp_spinlock_unlock(&odp_shm_tbl->lock); >>> -- >>> 1.8.5.1.163.gd7aced9 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 01/22/2015 06:33 PM, Mike Holmes wrote: > Can we add a validation case for this bug, Maxim can that be part of > this patch set, looks fairly simple to add as a second patch > There is test for that. See my v2 patch which includes Petris patches. What we need is just add script odp_shm_run which will turn on and turn off HP. > A very quick google suggests that a test case that calls "hugeadm > --explain" or one of the many other options and succeeds might be a > good way for the test case to know if it should run. Possibly better > is doing that test in automake so that it can set a flag for hugepage > which will then modify the list of test cases run. > I don't think that we need any tool for that with will do "echo 4096 > /proc/sys/vm/nr_hugepages". That can be in script. Maxim. > My quick google did not find a more portable way to know if HP was > available. > > Mike > > On 22 January 2015 at 10:25, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 01/22/2015 06:18 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > > -----Original Message----- > From: lng-odp-bounces@lists.linaro.org > <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp- > <mailto:lng-odp-> > bounces@lists.linaro.org > <mailto:bounces@lists.linaro.org>] On Behalf Of ext Maxim > Uvarov > Sent: Thursday, January 22, 2015 5:01 PM > To: lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > Subject: [lng-odp] [PATCH] linux-generic: shm fix unmap > for hugepages > > In case of hugepages unmap has to be done with address > aligned to > page size. Also unmap() has to be done for address > returned by mmap(), > not for aligned address after that. > > To reproduce original bug run: > echo 4096 > /proc/sys/vm/nr_hugepages > make check > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/odp_shared_memory.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_shared_memory.c > b/platform/linux- > generic/odp_shared_memory.c > index 99c5b40..51eba02 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -134,8 +134,14 @@ int odp_shm_free(odp_shm_t shm) > odp_spinlock_lock(&odp_shm_tbl->lock); > shm_block = &odp_shm_tbl->block[i]; > > +#ifdef MAP_HUGETLB > + /* round up alloc size by page */ > + alloc_size = (shm_block->size + > (shm_block->page_sz - 1)) > + & (-shm_block->page_sz); > > Could you rebase this on top of my two patches. And then move > this calculation to shm_reserve() side, so that the same size > gets allocated, saved into block->alloc_size, and the freed here. > > -Petri > > > Ok, I saw your patches after I wrote mine. Will rebase it on top. > > Maxim. > > > > +#else > alloc_size = shm_block->size + shm_block->align; > - ret = munmap(shm_block->addr, alloc_size); > +#endif > + ret = munmap(shm_block->addr_orig, alloc_size); > if (0 != ret) { > ODP_DBG("odp_shm_free: munmap failed\n"); > odp_spinlock_unlock(&odp_shm_tbl->lock); > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP
diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c index 99c5b40..51eba02 100644 --- a/platform/linux-generic/odp_shared_memory.c +++ b/platform/linux-generic/odp_shared_memory.c @@ -134,8 +134,14 @@ int odp_shm_free(odp_shm_t shm) odp_spinlock_lock(&odp_shm_tbl->lock); shm_block = &odp_shm_tbl->block[i]; +#ifdef MAP_HUGETLB + /* round up alloc size by page */ + alloc_size = (shm_block->size + (shm_block->page_sz - 1)) + & (-shm_block->page_sz); +#else alloc_size = shm_block->size + shm_block->align; - ret = munmap(shm_block->addr, alloc_size); +#endif + ret = munmap(shm_block->addr_orig, alloc_size); if (0 != ret) { ODP_DBG("odp_shm_free: munmap failed\n"); odp_spinlock_unlock(&odp_shm_tbl->lock);
In case of hugepages unmap has to be done with address aligned to page size. Also unmap() has to be done for address returned by mmap(), not for aligned address after that. To reproduce original bug run: echo 4096 > /proc/sys/vm/nr_hugepages make check Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/odp_shared_memory.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)