| Message ID | 20220209154301.42024-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftest/vm: Use correct PAGE_SHIFT value for ppc64 | expand |
On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: > Keep it simple by using a #define and limiting hugepage size to 2M. > This keeps the test simpler instead of dynamically finding the page size > and huge page size. > > Without this tests are broken w.r.t reading /proc/self/pagemap > > if (pread(pagemap_fd, ent, sizeof(ent), > (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) > err(2, "read pagemap"); > > Cc: Shuah Khan <shuah@kernel.org> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ > tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c > index 1436e1a9a3d3..8200328ff018 100644 > --- a/tools/testing/selftests/vm/ksm_tests.c > +++ b/tools/testing/selftests/vm/ksm_tests.c > @@ -22,8 +22,16 @@ > #define KSM_MERGE_ACROSS_NODES_DEFAULT true > #define MB (1ul << 20) > > +#ifdef __powerpc64__ > +#define PAGE_SHIFT 16 > +/* > + * This will only work with radix 2M hugepage size > + */ > +#define HPAGE_SHIFT 21 > +#else > #define PAGE_SHIFT 12 > #define HPAGE_SHIFT 21 > +#endif > > #define PAGE_SIZE (1 << PAGE_SHIFT) > #define HPAGE_SIZE (1 << HPAGE_SHIFT) > diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c > index 5e4c036f6ad3..f04c8aa4bcf6 100644 > --- a/tools/testing/selftests/vm/transhuge-stress.c > +++ b/tools/testing/selftests/vm/transhuge-stress.c > @@ -16,8 +16,16 @@ > #include <string.h> > #include <sys/mman.h> > > +#ifdef __powerpc64__ > +#define PAGE_SHIFT 16 > +/* > + * This will only work with radix 2M hugepage size > + */ > +#define HPAGE_SHIFT 21 Why not have this is in common code? > +#else > #define PAGE_SHIFT 12 > #define HPAGE_SHIFT 21 Same here. > +#endif > > #define PAGE_SIZE (1 << PAGE_SHIFT) > #define HPAGE_SIZE (1 << HPAGE_SHIFT) > Please cc linux-kselftest mailing list in the future. With the above fixed. Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
Shuah Khan <skhan@linuxfoundation.org> writes: > On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: >> Keep it simple by using a #define and limiting hugepage size to 2M. >> This keeps the test simpler instead of dynamically finding the page size >> and huge page size. >> >> Without this tests are broken w.r.t reading /proc/self/pagemap >> >> if (pread(pagemap_fd, ent, sizeof(ent), >> (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) >> err(2, "read pagemap"); >> >> Cc: Shuah Khan <shuah@kernel.org> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ >> tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c >> index 1436e1a9a3d3..8200328ff018 100644 >> --- a/tools/testing/selftests/vm/ksm_tests.c >> +++ b/tools/testing/selftests/vm/ksm_tests.c >> @@ -22,8 +22,16 @@ >> #define KSM_MERGE_ACROSS_NODES_DEFAULT true >> #define MB (1ul << 20) >> >> +#ifdef __powerpc64__ >> +#define PAGE_SHIFT 16 >> +/* >> + * This will only work with radix 2M hugepage size >> + */ >> +#define HPAGE_SHIFT 21 >> +#else >> #define PAGE_SHIFT 12 >> #define HPAGE_SHIFT 21 >> +#endif >> >> #define PAGE_SIZE (1 << PAGE_SHIFT) >> #define HPAGE_SIZE (1 << HPAGE_SHIFT) >> diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c >> index 5e4c036f6ad3..f04c8aa4bcf6 100644 >> --- a/tools/testing/selftests/vm/transhuge-stress.c >> +++ b/tools/testing/selftests/vm/transhuge-stress.c >> @@ -16,8 +16,16 @@ >> #include <string.h> >> #include <sys/mman.h> >> >> +#ifdef __powerpc64__ >> +#define PAGE_SHIFT 16 >> +/* >> + * This will only work with radix 2M hugepage size >> + */ >> +#define HPAGE_SHIFT 21 > > Why not have this is in common code? Can you suggest where I can move that. We also have helper functions like allocate_transhuge() duplicated between tests. I didn't find libutil.a or anything similar supported by the selftets build. > >> +#else >> #define PAGE_SHIFT 12 >> #define HPAGE_SHIFT 21 > > Same here. > >> +#endif >> >> #define PAGE_SIZE (1 << PAGE_SHIFT) >> #define HPAGE_SIZE (1 << HPAGE_SHIFT) >> > > Please cc linux-kselftest mailing list in the future. > > With the above fixed. > > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> > > thanks, > -- Shuah -aneesh
Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes: > Shuah Khan <skhan@linuxfoundation.org> writes: > >> On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: >> --- a/tools/testing/selftests/vm/transhuge-stress.c >>> +++ b/tools/testing/selftests/vm/transhuge-stress.c >>> @@ -16,8 +16,16 @@ >>> #include <string.h> >>> #include <sys/mman.h> >>> >>> +#ifdef __powerpc64__ >>> +#define PAGE_SHIFT 16 >>> +/* >>> + * This will only work with radix 2M hugepage size >>> + */ >>> +#define HPAGE_SHIFT 21 >> >> Why not have this is in common code? > > Can you suggest where I can move that. We also have helper functions > like allocate_transhuge() duplicated between tests. I didn't find > libutil.a or anything similar supported by the selftets build. > >> >>> +#else >>> #define PAGE_SHIFT 12 >>> #define HPAGE_SHIFT 21 >> I can do a util.h for vm related test as below. commit 378fa2d73f1255d3045023875dd00d5b8486440e Author: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Date: Thu Feb 10 10:22:27 2022 +0530 selftest/vm: Add util.h and and move helper functions there Avoid code duplication by adding util.h Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c index 8200328ff018..fd85f15869d1 100644 --- a/tools/testing/selftests/vm/ksm_tests.c +++ b/tools/testing/selftests/vm/ksm_tests.c @@ -12,6 +12,7 @@ #include "../kselftest.h" #include "../../../../include/vdso/time64.h" +#include "util.h" #define KSM_SYSFS_PATH "/sys/kernel/mm/ksm/" #define KSM_FP(s) (KSM_SYSFS_PATH s) @@ -22,23 +23,6 @@ #define KSM_MERGE_ACROSS_NODES_DEFAULT true #define MB (1ul << 20) -#ifdef __powerpc64__ -#define PAGE_SHIFT 16 -/* - * This will only work with radix 2M hugepage size - */ -#define HPAGE_SHIFT 21 -#else -#define PAGE_SHIFT 12 -#define HPAGE_SHIFT 21 -#endif - -#define PAGE_SIZE (1 << PAGE_SHIFT) -#define HPAGE_SIZE (1 << HPAGE_SHIFT) - -#define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) -#define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1)) - struct ksm_sysfs { unsigned long max_page_sharing; unsigned long merge_across_nodes; @@ -464,34 +448,6 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a return KSFT_FAIL; } -int64_t allocate_transhuge(void *ptr, int pagemap_fd) -{ - uint64_t ent[2]; - - /* drop pmd */ - if (mmap(ptr, HPAGE_SIZE, PROT_READ | PROT_WRITE, - MAP_FIXED | MAP_ANONYMOUS | - MAP_NORESERVE | MAP_PRIVATE, -1, 0) != ptr) - errx(2, "mmap transhuge"); - - if (madvise(ptr, HPAGE_SIZE, MADV_HUGEPAGE)) - err(2, "MADV_HUGEPAGE"); - - /* allocate transparent huge page */ - *(volatile void **)ptr = ptr; - - if (pread(pagemap_fd, ent, sizeof(ent), - (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) - err(2, "read pagemap"); - - if (PAGEMAP_PRESENT(ent[0]) && PAGEMAP_PRESENT(ent[1]) && - PAGEMAP_PFN(ent[0]) + 1 == PAGEMAP_PFN(ent[1]) && - !(PAGEMAP_PFN(ent[0]) & ((1 << (HPAGE_SHIFT - PAGE_SHIFT)) - 1))) - return PAGEMAP_PFN(ent[0]); - - return -1; -} - static int ksm_merge_hugepages_time(int mapping, int prot, int timeout, size_t map_size) { void *map_ptr, *map_ptr_orig; diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c index f04c8aa4bcf6..0da4aa10746a 100644 --- a/tools/testing/selftests/vm/transhuge-stress.c +++ b/tools/testing/selftests/vm/transhuge-stress.c @@ -16,52 +16,7 @@ #include <string.h> #include <sys/mman.h> -#ifdef __powerpc64__ -#define PAGE_SHIFT 16 -/* - * This will only work with radix 2M hugepage size - */ -#define HPAGE_SHIFT 21 -#else -#define PAGE_SHIFT 12 -#define HPAGE_SHIFT 21 -#endif - -#define PAGE_SIZE (1 << PAGE_SHIFT) -#define HPAGE_SIZE (1 << HPAGE_SHIFT) - -#define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) -#define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1)) - -int pagemap_fd; - -int64_t allocate_transhuge(void *ptr) -{ - uint64_t ent[2]; - - /* drop pmd */ - if (mmap(ptr, HPAGE_SIZE, PROT_READ | PROT_WRITE, - MAP_FIXED | MAP_ANONYMOUS | - MAP_NORESERVE | MAP_PRIVATE, -1, 0) != ptr) - errx(2, "mmap transhuge"); - - if (madvise(ptr, HPAGE_SIZE, MADV_HUGEPAGE)) - err(2, "MADV_HUGEPAGE"); - - /* allocate transparent huge page */ - *(volatile void **)ptr = ptr; - - if (pread(pagemap_fd, ent, sizeof(ent), - (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) - err(2, "read pagemap"); - - if (PAGEMAP_PRESENT(ent[0]) && PAGEMAP_PRESENT(ent[1]) && - PAGEMAP_PFN(ent[0]) + 1 == PAGEMAP_PFN(ent[1]) && - !(PAGEMAP_PFN(ent[0]) & ((1 << (HPAGE_SHIFT - PAGE_SHIFT)) - 1))) - return PAGEMAP_PFN(ent[0]); - - return -1; -} +#include "util.h" int main(int argc, char **argv) { @@ -71,6 +26,7 @@ int main(int argc, char **argv) double s; uint8_t *map; size_t map_len; + int pagemap_fd; ram = sysconf(_SC_PHYS_PAGES); if (ram > SIZE_MAX / sysconf(_SC_PAGESIZE) / 4) @@ -117,7 +73,7 @@ int main(int argc, char **argv) for (p = ptr; p < ptr + len; p += HPAGE_SIZE) { int64_t pfn; - pfn = allocate_transhuge(p); + pfn = allocate_transhuge(p, pagemap_fd); if (pfn < 0) { nr_failed++; diff --git a/tools/testing/selftests/vm/util.h b/tools/testing/selftests/vm/util.h new file mode 100644 index 000000000000..1461a96f0bc0 --- /dev/null +++ b/tools/testing/selftests/vm/util.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __KSELFTEST_VM_UTIL_H +#define __KSELFTEST_VM_UTIL_H + +#include <sys/mman.h> +#include <err.h> + +#ifdef __powerpc64__ +#define PAGE_SHIFT 16 +/* + * This will only work with radix 2M hugepage size + */ +#define HPAGE_SHIFT 21 +#else +#define PAGE_SHIFT 12 +#define HPAGE_SHIFT 21 +#endif + +#define PAGE_SIZE (1 << PAGE_SHIFT) +#define HPAGE_SIZE (1 << HPAGE_SHIFT) + +#define PAGEMAP_PRESENT(ent) (((ent) & (1ull << 63)) != 0) +#define PAGEMAP_PFN(ent) ((ent) & ((1ull << 55) - 1)) + + +static inline int64_t allocate_transhuge(void *ptr, int pagemap_fd) +{ + uint64_t ent[2]; + + /* drop pmd */ + if (mmap(ptr, HPAGE_SIZE, PROT_READ | PROT_WRITE, + MAP_FIXED | MAP_ANONYMOUS | + MAP_NORESERVE | MAP_PRIVATE, -1, 0) != ptr) + errx(2, "mmap transhuge"); + + if (madvise(ptr, HPAGE_SIZE, MADV_HUGEPAGE)) + err(2, "MADV_HUGEPAGE"); + + /* allocate transparent huge page */ + *(volatile void **)ptr = ptr; + + if (pread(pagemap_fd, ent, sizeof(ent), + (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) + err(2, "read pagemap"); + + if (PAGEMAP_PRESENT(ent[0]) && PAGEMAP_PRESENT(ent[1]) && + PAGEMAP_PFN(ent[0]) + 1 == PAGEMAP_PFN(ent[1]) && + !(PAGEMAP_PFN(ent[0]) & ((1 << (HPAGE_SHIFT - PAGE_SHIFT)) - 1))) + return PAGEMAP_PFN(ent[0]); + + return -1; +} + +#endif
On 2/9/22 9:12 PM, Aneesh Kumar K.V wrote: > Shuah Khan <skhan@linuxfoundation.org> writes: > >> On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: >>> Keep it simple by using a #define and limiting hugepage size to 2M. >>> This keeps the test simpler instead of dynamically finding the page size >>> and huge page size. >>> >>> Without this tests are broken w.r.t reading /proc/self/pagemap >>> >>> if (pread(pagemap_fd, ent, sizeof(ent), >>> (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) >>> err(2, "read pagemap"); >>> >>> Cc: Shuah Khan <shuah@kernel.org> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> --- >>> tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ >>> tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ >>> 2 files changed, 16 insertions(+) >>> >>> diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c >>> index 1436e1a9a3d3..8200328ff018 100644 >>> --- a/tools/testing/selftests/vm/ksm_tests.c >>> +++ b/tools/testing/selftests/vm/ksm_tests.c >>> @@ -22,8 +22,16 @@ >>> #define KSM_MERGE_ACROSS_NODES_DEFAULT true >>> #define MB (1ul << 20) >>> >>> +#ifdef __powerpc64__ >>> +#define PAGE_SHIFT 16 >>> +/* >>> + * This will only work with radix 2M hugepage size >>> + */ >>> +#define HPAGE_SHIFT 21 >>> +#else >>> #define PAGE_SHIFT 12 >>> #define HPAGE_SHIFT 21 >>> +#endif >>> >>> #define PAGE_SIZE (1 << PAGE_SHIFT) >>> #define HPAGE_SIZE (1 << HPAGE_SHIFT) >>> diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c >>> index 5e4c036f6ad3..f04c8aa4bcf6 100644 >>> --- a/tools/testing/selftests/vm/transhuge-stress.c >>> +++ b/tools/testing/selftests/vm/transhuge-stress.c >>> @@ -16,8 +16,16 @@ >>> #include <string.h> >>> #include <sys/mman.h> >>> >>> +#ifdef __powerpc64__ >>> +#define PAGE_SHIFT 16 >>> +/* >>> + * This will only work with radix 2M hugepage size >>> + */ >>> +#define HPAGE_SHIFT 21 >> >> Why not have this is in common code? > > Can you suggest where I can move that. We also have helper functions > like allocate_transhuge() duplicated between tests. I didn't find > libutil.a or anything similar supported by the selftets build. > >> I noticed that HPAGE_SHIFT is defined in #ifdef __powerpc64__ block as well as #else. I am asking is it necessary to be part of both blocks. +#ifdef __powerpc64__ +#define PAGE_SHIFT 16 +/* + * This will only work with radix 2M hugepage size + */ +#define HPAGE_SHIFT 21 --- this one +#else #define PAGE_SHIFT 12 #define HPAGE_SHIFT 21 --- this one +#endif Hope this helps. thanks, -- Shuah
On 2/10/22 20:09, Shuah Khan wrote: > On 2/9/22 9:12 PM, Aneesh Kumar K.V wrote: >> Shuah Khan <skhan@linuxfoundation.org> writes: >> >>> On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: >>>> Keep it simple by using a #define and limiting hugepage size to 2M. >>>> This keeps the test simpler instead of dynamically finding the page >>>> size >>>> and huge page size. >>>> >>>> Without this tests are broken w.r.t reading /proc/self/pagemap >>>> >>>> if (pread(pagemap_fd, ent, sizeof(ent), >>>> (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) >>>> err(2, "read pagemap"); >>>> >>>> Cc: Shuah Khan <shuah@kernel.org> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>> --- >>>> tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ >>>> tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ >>>> 2 files changed, 16 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/vm/ksm_tests.c >>>> b/tools/testing/selftests/vm/ksm_tests.c >>>> index 1436e1a9a3d3..8200328ff018 100644 >>>> --- a/tools/testing/selftests/vm/ksm_tests.c >>>> +++ b/tools/testing/selftests/vm/ksm_tests.c >>>> @@ -22,8 +22,16 @@ >>>> #define KSM_MERGE_ACROSS_NODES_DEFAULT true >>>> #define MB (1ul << 20) >>>> +#ifdef __powerpc64__ >>>> +#define PAGE_SHIFT 16 >>>> +/* >>>> + * This will only work with radix 2M hugepage size >>>> + */ >>>> +#define HPAGE_SHIFT 21 >>>> +#else >>>> #define PAGE_SHIFT 12 >>>> #define HPAGE_SHIFT 21 >>>> +#endif >>>> #define PAGE_SIZE (1 << PAGE_SHIFT) >>>> #define HPAGE_SIZE (1 << HPAGE_SHIFT) >>>> diff --git a/tools/testing/selftests/vm/transhuge-stress.c >>>> b/tools/testing/selftests/vm/transhuge-stress.c >>>> index 5e4c036f6ad3..f04c8aa4bcf6 100644 >>>> --- a/tools/testing/selftests/vm/transhuge-stress.c >>>> +++ b/tools/testing/selftests/vm/transhuge-stress.c >>>> @@ -16,8 +16,16 @@ >>>> #include <string.h> >>>> #include <sys/mman.h> >>>> +#ifdef __powerpc64__ >>>> +#define PAGE_SHIFT 16 >>>> +/* >>>> + * This will only work with radix 2M hugepage size >>>> + */ >>>> +#define HPAGE_SHIFT 21 >>> >>> Why not have this is in common code? >> >> Can you suggest where I can move that. We also have helper functions >> like allocate_transhuge() duplicated between tests. I didn't find >> libutil.a or anything similar supported by the selftets build. >> >>> > > I noticed that HPAGE_SHIFT is defined in #ifdef __powerpc64__ block > as well as #else. I am asking is it necessary to be part of both > blocks. > > +#ifdef __powerpc64__ > +#define PAGE_SHIFT 16 > +/* > + * This will only work with radix 2M hugepage size > + */ > +#define HPAGE_SHIFT 21 --- this one > +#else > #define PAGE_SHIFT 12 > #define HPAGE_SHIFT 21 --- this one > +#endif > The reason I did that was to add the comment which is relevant only for ppc64. ppc64 supports two hugepage sizes, 2M and 16M. The test won't work correctly with 16M hugepage size. We do have other tests in selftest/vm/ with similar restrictions. -aneesh
On 2/10/22 8:03 AM, Aneesh Kumar K V wrote: > On 2/10/22 20:09, Shuah Khan wrote: >> On 2/9/22 9:12 PM, Aneesh Kumar K.V wrote: >>> Shuah Khan <skhan@linuxfoundation.org> writes: >>> >>>> On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote: >>>>> Keep it simple by using a #define and limiting hugepage size to 2M. >>>>> This keeps the test simpler instead of dynamically finding the page size >>>>> and huge page size. >>>>> >>>>> Without this tests are broken w.r.t reading /proc/self/pagemap >>>>> >>>>> if (pread(pagemap_fd, ent, sizeof(ent), >>>>> (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) >>>>> err(2, "read pagemap"); >>>>> >>>>> Cc: Shuah Khan <shuah@kernel.org> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>>> --- >>>>> tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ >>>>> tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ >>>>> 2 files changed, 16 insertions(+) >>>>> >>>>> diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c >>>>> index 1436e1a9a3d3..8200328ff018 100644 >>>>> --- a/tools/testing/selftests/vm/ksm_tests.c >>>>> +++ b/tools/testing/selftests/vm/ksm_tests.c >>>>> @@ -22,8 +22,16 @@ >>>>> #define KSM_MERGE_ACROSS_NODES_DEFAULT true >>>>> #define MB (1ul << 20) >>>>> +#ifdef __powerpc64__ >>>>> +#define PAGE_SHIFT 16 >>>>> +/* >>>>> + * This will only work with radix 2M hugepage size >>>>> + */ >>>>> +#define HPAGE_SHIFT 21 >>>>> +#else >>>>> #define PAGE_SHIFT 12 >>>>> #define HPAGE_SHIFT 21 >>>>> +#endif >>>>> #define PAGE_SIZE (1 << PAGE_SHIFT) >>>>> #define HPAGE_SIZE (1 << HPAGE_SHIFT) >>>>> diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c >>>>> index 5e4c036f6ad3..f04c8aa4bcf6 100644 >>>>> --- a/tools/testing/selftests/vm/transhuge-stress.c >>>>> +++ b/tools/testing/selftests/vm/transhuge-stress.c >>>>> @@ -16,8 +16,16 @@ >>>>> #include <string.h> >>>>> #include <sys/mman.h> >>>>> +#ifdef __powerpc64__ >>>>> +#define PAGE_SHIFT 16 >>>>> +/* >>>>> + * This will only work with radix 2M hugepage size >>>>> + */ >>>>> +#define HPAGE_SHIFT 21 >>>> >>>> Why not have this is in common code? >>> >>> Can you suggest where I can move that. We also have helper functions >>> like allocate_transhuge() duplicated between tests. I didn't find >>> libutil.a or anything similar supported by the selftets build. >>> >>>> >> >> I noticed that HPAGE_SHIFT is defined in #ifdef __powerpc64__ block >> as well as #else. I am asking is it necessary to be part of both >> blocks. >> >> +#ifdef __powerpc64__ >> +#define PAGE_SHIFT 16 >> +/* >> + * This will only work with radix 2M hugepage size >> + */ >> +#define HPAGE_SHIFT 21 --- this one >> +#else >> #define PAGE_SHIFT 12 >> #define HPAGE_SHIFT 21 --- this one >> +#endif >> > > > The reason I did that was to add the comment which is relevant only for ppc64. ppc64 supports two hugepage sizes, 2M and 16M. The test won't work correctly with 16M hugepage size. We do have other tests in selftest/vm/ with similar restrictions. > > Right. You don't have to duplicate code for the comment. You can add the comment and then clarify in the comment that it is only relevant to ppc64. This way the comment is clear and we can avoid duplicate code that makes it hard to maintain in the future. thanks, -- Shuah
diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c index 1436e1a9a3d3..8200328ff018 100644 --- a/tools/testing/selftests/vm/ksm_tests.c +++ b/tools/testing/selftests/vm/ksm_tests.c @@ -22,8 +22,16 @@ #define KSM_MERGE_ACROSS_NODES_DEFAULT true #define MB (1ul << 20) +#ifdef __powerpc64__ +#define PAGE_SHIFT 16 +/* + * This will only work with radix 2M hugepage size + */ +#define HPAGE_SHIFT 21 +#else #define PAGE_SHIFT 12 #define HPAGE_SHIFT 21 +#endif #define PAGE_SIZE (1 << PAGE_SHIFT) #define HPAGE_SIZE (1 << HPAGE_SHIFT) diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c index 5e4c036f6ad3..f04c8aa4bcf6 100644 --- a/tools/testing/selftests/vm/transhuge-stress.c +++ b/tools/testing/selftests/vm/transhuge-stress.c @@ -16,8 +16,16 @@ #include <string.h> #include <sys/mman.h> +#ifdef __powerpc64__ +#define PAGE_SHIFT 16 +/* + * This will only work with radix 2M hugepage size + */ +#define HPAGE_SHIFT 21 +#else #define PAGE_SHIFT 12 #define HPAGE_SHIFT 21 +#endif #define PAGE_SIZE (1 << PAGE_SHIFT) #define HPAGE_SIZE (1 << HPAGE_SHIFT)
Keep it simple by using a #define and limiting hugepage size to 2M. This keeps the test simpler instead of dynamically finding the page size and huge page size. Without this tests are broken w.r.t reading /proc/self/pagemap if (pread(pagemap_fd, ent, sizeof(ent), (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent)) err(2, "read pagemap"); Cc: Shuah Khan <shuah@kernel.org> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- tools/testing/selftests/vm/ksm_tests.c | 8 ++++++++ tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++ 2 files changed, 16 insertions(+)