Message ID | 20231003133703.98552-1-geetika@linux.ibm.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Migrating the libhugetlbfs/testcases/straddle_4GB.c | expand |
Hi Geetika, Please have look at my comments at your previous patch [1] [2], these changes apply a lot for your patches as well. [1] https://lore.kernel.org/ltp/20231128111024.GA364870@pevik/ [2] https://lore.kernel.org/ltp/20231128112254.GA367506@pevik/ > Test Description: > This test tries to allocate hugepages to cover a memory range > that straddles the 4GB boundary. > Signed-off-by: Geetika <geetika@linux.ibm.com> > --- > runtest/hugetlb | 1 + > testcases/kernel/mem/.gitignore | 1 + > .../kernel/mem/hugetlb/hugemmap/hugemmap40.c | 145 ++++++++++++++++++ > testcases/kernel/mem/hugetlb/lib/hugetlb.c | 42 +++++ > 4 files changed, 189 insertions(+) > create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c > diff --git a/runtest/hugetlb b/runtest/hugetlb > index 299c07ac9..da37dc815 100644 > --- a/runtest/hugetlb > +++ b/runtest/hugetlb > @@ -35,6 +35,7 @@ hugemmap29 hugemmap29 > hugemmap30 hugemmap30 > hugemmap31 hugemmap31 > hugemmap32 hugemmap32 > +hugemmap40 hugemmap40 > hugemmap05_1 hugemmap05 -m > hugemmap05_2 hugemmap05 -s > hugemmap05_3 hugemmap05 -s -m > diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore > index 7258489ed..dd1858a4a 100644 > --- a/testcases/kernel/mem/.gitignore > +++ b/testcases/kernel/mem/.gitignore > @@ -34,6 +34,7 @@ > /hugetlb/hugemmap/hugemmap30 > /hugetlb/hugemmap/hugemmap31 > /hugetlb/hugemmap/hugemmap32 > +/hugetlb/hugemmap/hugemmap40 > /hugetlb/hugeshmat/hugeshmat01 > /hugetlb/hugeshmat/hugeshmat02 > /hugetlb/hugeshmat/hugeshmat03 > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c > new file mode 100644 > index 000000000..1ba070831 > --- /dev/null > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c > @@ -0,0 +1,145 @@ > +// SPDX-License-Identifier: LGPL-2.1-or-later > +/* > + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation. > + * Copyright (C) 2006 Hugh Dickins <hugh@veritas.com> > + */ > + > +/*\ > + * [Description] > + * This test tries to allocate hugepages to cover a memory range > + * that straddles the 4GB boundary. > + * Scenario 1 : mmap without MAP_FIXED > + * Scenario 2 : mmap with MAP_FIXED This will be badly formatted (it will not be list, but inline). How about: /*\ * [Description] * * Test tries to allocate hugepages to cover a memory range that straddles the * 4GB boundary, using mmap(2) with and without MAP_FIXED. */ > + */ > + > +#define MAPS_BUF_SZ 4096 > +#define FOURGB (1UL << 32) > +#define MNTPOINT "hugetlbfs/" > +#define HUGETLBFS_MAGIC 0x958458f6 Could you please add this magic definition to include/tst_fs.h (as a separate patch), we store all magic there. > +#define _LARGEFILE64_SOURCE /* Need this for statfs64 */ We would probably define it in Makefile hugemmap40: CFLAGS += -D_LARGEFILE64_SOURCE > + > +#include "hugetlb.h" > + > +static long hpage_size; > +static int fd = -1; > +static unsigned long straddle_addr; > + > +static int test_addr_huge(void *p) > +{ > + char name[256]; > + char *dirend; > + int ret; > + struct statfs64 sb; > + > + ret = read_maps((unsigned long)p, name); > + if (ret < 0) > + return ret; > + if (ret == 0) { > + tst_res(TINFO, "Couldn't find address %p in /proc/self/maps\n", p); Please remove "\n", it's already added in tst_res(). > + return -1; > + } > + /* looks like a filename? */ > + if (name[0] != '/') > + return 0; > + > + /* Truncate the filename portion */ > + > + dirend = strrchr(name, '/'); > + if (dirend && dirend > name) > + *dirend = '\0'; > + > + ret = statfs64(name, &sb); > + if (ret) > + return -1; > + > + return (sb.f_type == HUGETLBFS_MAGIC); > +} > + > +static void run_test(void) > +{ > + void *p; > + > + /* We first try to get the mapping without MAP_FIXED */ > + tst_res(TINFO, "Mapping without MAP_FIXED at %lx...", straddle_addr); > + p = mmap((void *)straddle_addr, 2*hpage_size, PROT_READ|PROT_WRITE, > + MAP_SHARED, fd, 0); > + if (p == (void *)straddle_addr) { > + /* These tests irrelevant if we didn't get the straddle address*/ > + if (test_addr_huge(p) != 1) { > + tst_brk(TFAIL, "1st Mapped address is not hugepage"); > + goto windup; > + } > + if (test_addr_huge(p + hpage_size) != 1) { > + tst_brk(TFAIL, "2nd Mapped address is not hugepage"); > + goto windup; > + } > + memset(p, 0, hpage_size); > + memset(p + hpage_size, 0, hpage_size); > + tst_res(TPASS, "Mapping without MAP_FIXED at %lx... completed", straddle_addr); > + } else { > + tst_res(TINFO, "Got %p instead, never mind, let's move to mapping with MAP_FIXED\n", p); > + munmap(p, 2*hpage_size); > + } > + tst_res(TINFO, "Mapping with MAP_FIXED at %lx...", straddle_addr); Maybe use .tcnt = 2, in struct tst_test and separate these 2 cases into it's wonf functions? You would either use: static void run_test(unsigned int n) With that you would reduce code duplicity and make test function smaller. Also, sometimes we use test struct (pointer to the function and description, see e.g. testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c). ... > +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.c Maybe add this change as a separate commit? Kind regards, Petr > @@ -130,3 +130,45 @@ int do_readback(void *p, size_t size, char *desc) > } > return 0; > } > + > +#define MAPS_BUF_SZ 4096 > +int read_maps(unsigned long addr, char *buf) > +{ > + FILE *f; > + char line[MAPS_BUF_SZ]; > + char *tmp; > + > + f = fopen("/proc/self/maps", "r"); > + if (!f) { > + tst_res(TFAIL, "Failed to open /proc/self/maps: %s\n", strerror(errno)); > + return -1; > + } > + > + while (1) { > + unsigned long start, end, off, ino; > + int ret; > + > + tmp = fgets(line, MAPS_BUF_SZ, f); > + if (!tmp) > + break; > + > + buf[0] = '\0'; > + ret = sscanf(line, "%lx-%lx %*s %lx %*s %ld %255s", > + &start, &end, &off, &ino, > + buf); > + if ((ret < 4) || (ret > 5)) { > + tst_res(TFAIL, "Couldn't parse /proc/self/maps line: %s\n", > + line); > + fclose(f); > + return -1; > + } > + > + if ((start <= addr) && (addr < end)) { > + fclose(f); > + return 1; > + } > + } > + > + fclose(f); > + return 0; > +}
Hi Petr, On 28/11/23 5:20 pm, Petr Vorel wrote: > Hi Geetika, > > Please have look at my comments at your previous patch [1] [2], these changes > apply a lot for your patches as well. > > [1]https://lore.kernel.org/ltp/20231128111024.GA364870@pevik/ > [2]https://lore.kernel.org/ltp/20231128112254.GA367506@pevik/ > Noted. I will apply whatever changes are applicable to this testcase in the next version. ... >> + >> +/*\ >> + * [Description] >> + * This test tries to allocate hugepages to cover a memory range >> + * that straddles the 4GB boundary. >> + * Scenario 1 : mmap without MAP_FIXED >> + * Scenario 2 : mmap with MAP_FIXED > This will be badly formatted (it will not be list, but inline). > > How about: > > /*\ > * [Description] > * > * Test tries to allocate hugepages to cover a memory range that straddles the > * 4GB boundary, using mmap(2) with and without MAP_FIXED. > */ >> + */ >> + >> +#define MAPS_BUF_SZ 4096 >> +#define FOURGB (1UL << 32) >> +#define MNTPOINT "hugetlbfs/" >> +#define HUGETLBFS_MAGIC 0x958458f6 > Could you please add this magic definition to include/tst_fs.h > (as a separate patch), we store all magic there. > >> +#define _LARGEFILE64_SOURCE /* Need this for statfs64 */ > We would probably define it in Makefile > > hugemmap40: CFLAGS += -D_LARGEFILE64_SOURCE Noted. I will apply above changes with the next version. ... > + > +static void run_test(void) > +{ > + void *p; > + > + /* We first try to get the mapping without MAP_FIXED */ > + tst_res(TINFO, "Mapping without MAP_FIXED at %lx...", straddle_addr); > + p = mmap((void *)straddle_addr, 2*hpage_size, PROT_READ|PROT_WRITE, > + MAP_SHARED, fd, 0); > + if (p == (void *)straddle_addr) { > + /* These tests irrelevant if we didn't get the straddle address*/ > + if (test_addr_huge(p) != 1) { > + tst_brk(TFAIL, "1st Mapped address is not hugepage"); > + goto windup; > + } > + if (test_addr_huge(p + hpage_size) != 1) { > + tst_brk(TFAIL, "2nd Mapped address is not hugepage"); > + goto windup; > + } > + memset(p, 0, hpage_size); > + memset(p + hpage_size, 0, hpage_size); > + tst_res(TPASS, "Mapping without MAP_FIXED at %lx... completed", straddle_addr); > + } else { > + tst_res(TINFO, "Got %p instead, never mind, let's move to mapping with MAP_FIXED\n", p); > + munmap(p, 2*hpage_size); > + } > + tst_res(TINFO, "Mapping with MAP_FIXED at %lx...", straddle_addr); > Maybe use .tcnt = 2, in struct tst_test and separate these 2 cases into it's > wonf functions? > > You would either use: > static void run_test(unsigned int n) > > With that you would reduce code duplicity and make test function smaller. > > Also, sometimes we use test struct (pointer to the function and description, see > e.g. testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c). > > ... Can you suggest how we can handle the test exit upon failure inside one of test cases then? Currently I am handling it by using label windup placed at the end of function. In one of the other tests there was feedback to not use exit(0); then how can we exit easily upon failure from one of the testcase? Thanks & Regards, Geetika
diff --git a/runtest/hugetlb b/runtest/hugetlb index 299c07ac9..da37dc815 100644 --- a/runtest/hugetlb +++ b/runtest/hugetlb @@ -35,6 +35,7 @@ hugemmap29 hugemmap29 hugemmap30 hugemmap30 hugemmap31 hugemmap31 hugemmap32 hugemmap32 +hugemmap40 hugemmap40 hugemmap05_1 hugemmap05 -m hugemmap05_2 hugemmap05 -s hugemmap05_3 hugemmap05 -s -m diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore index 7258489ed..dd1858a4a 100644 --- a/testcases/kernel/mem/.gitignore +++ b/testcases/kernel/mem/.gitignore @@ -34,6 +34,7 @@ /hugetlb/hugemmap/hugemmap30 /hugetlb/hugemmap/hugemmap31 /hugetlb/hugemmap/hugemmap32 +/hugetlb/hugemmap/hugemmap40 /hugetlb/hugeshmat/hugeshmat01 /hugetlb/hugeshmat/hugeshmat02 /hugetlb/hugeshmat/hugeshmat03 diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c new file mode 100644 index 000000000..1ba070831 --- /dev/null +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +/* + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation. + * Copyright (C) 2006 Hugh Dickins <hugh@veritas.com> + */ + +/*\ + * [Description] + * This test tries to allocate hugepages to cover a memory range + * that straddles the 4GB boundary. + * Scenario 1 : mmap without MAP_FIXED + * Scenario 2 : mmap with MAP_FIXED + * + */ + +#define MAPS_BUF_SZ 4096 +#define FOURGB (1UL << 32) +#define MNTPOINT "hugetlbfs/" +#define HUGETLBFS_MAGIC 0x958458f6 +#define _LARGEFILE64_SOURCE /* Need this for statfs64 */ + +#include "hugetlb.h" + +static long hpage_size; +static int fd = -1; +static unsigned long straddle_addr; + +static int test_addr_huge(void *p) +{ + char name[256]; + char *dirend; + int ret; + struct statfs64 sb; + + ret = read_maps((unsigned long)p, name); + if (ret < 0) + return ret; + if (ret == 0) { + tst_res(TINFO, "Couldn't find address %p in /proc/self/maps\n", p); + return -1; + } + /* looks like a filename? */ + if (name[0] != '/') + return 0; + + /* Truncate the filename portion */ + + dirend = strrchr(name, '/'); + if (dirend && dirend > name) + *dirend = '\0'; + + ret = statfs64(name, &sb); + if (ret) + return -1; + + return (sb.f_type == HUGETLBFS_MAGIC); +} + +static void run_test(void) +{ + void *p; + + /* We first try to get the mapping without MAP_FIXED */ + tst_res(TINFO, "Mapping without MAP_FIXED at %lx...", straddle_addr); + p = mmap((void *)straddle_addr, 2*hpage_size, PROT_READ|PROT_WRITE, + MAP_SHARED, fd, 0); + if (p == (void *)straddle_addr) { + /* These tests irrelevant if we didn't get the straddle address*/ + if (test_addr_huge(p) != 1) { + tst_brk(TFAIL, "1st Mapped address is not hugepage"); + goto windup; + } + if (test_addr_huge(p + hpage_size) != 1) { + tst_brk(TFAIL, "2nd Mapped address is not hugepage"); + goto windup; + } + memset(p, 0, hpage_size); + memset(p + hpage_size, 0, hpage_size); + tst_res(TPASS, "Mapping without MAP_FIXED at %lx... completed", straddle_addr); + } else { + tst_res(TINFO, "Got %p instead, never mind, let's move to mapping with MAP_FIXED\n", p); + munmap(p, 2*hpage_size); + } + tst_res(TINFO, "Mapping with MAP_FIXED at %lx...", straddle_addr); + p = mmap((void *)straddle_addr, 2*hpage_size, PROT_READ|PROT_WRITE, + MAP_SHARED|MAP_FIXED, fd, 0); + if (p == MAP_FAILED) { + /* this area crosses last low slice and first high slice */ + unsigned long below_start = FOURGB - 256L*1024*1024; + unsigned long above_end = 1024L*1024*1024*1024; + + if (range_is_mapped(below_start, above_end) == 1) { + tst_res(TINFO, "region (4G-256M)-1T is not free"); + tst_res(TINFO, "mmap() failed: %s\n", strerror(errno)); + tst_res(TWARN, "Pass Inconclusive!"); + goto windup; + } else + tst_res(TFAIL, "mmap() FIXED failed: %s\n", strerror(errno)); + } + if (p != (void *)straddle_addr) { + tst_res(TINFO, "got %p instead\n", p); + tst_res(TFAIL, "Wrong address with MAP_FIXED"); + goto windup; + } + + if (test_addr_huge(p) != 1) { + tst_brk(TFAIL, "1st Mapped address is not hugepage"); + goto windup; + } + if (test_addr_huge(p + hpage_size) != 1) { + tst_brk(TFAIL, "2nd Mapped address is not hugepage"); + goto windup; + } + memset(p, 0, hpage_size); + memset(p + hpage_size, 0, hpage_size); + tst_res(TPASS, "Mapping with MAP_FIXED at %lx... completed", straddle_addr); + +windup: + SAFE_MUNMAP(p, 2*hpage_size); +} + +static void setup(void) +{ + straddle_addr = FOURGB - hpage_size; + hpage_size = tst_get_hugepage_size(); + fd = tst_creat_unlinked(MNTPOINT, 0); + if (hpage_size > FOURGB) + tst_brk(TCONF, "Huge page size is too large!"); +} + +static void cleanup(void) +{ + if (fd >= 0) + SAFE_CLOSE(fd); +} + +static struct tst_test test = { + .needs_root = 1, + .mntpoint = MNTPOINT, + .needs_hugetlbfs = 1, + .hugepages = {2, TST_NEEDS}, + .setup = setup, + .cleanup = cleanup, + .test_all = run_test, +}; diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.c b/testcases/kernel/mem/hugetlb/lib/hugetlb.c index 43a677ce9..39f101517 100644 --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.c +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.c @@ -130,3 +130,45 @@ int do_readback(void *p, size_t size, char *desc) } return 0; } + +#define MAPS_BUF_SZ 4096 +int read_maps(unsigned long addr, char *buf) +{ + FILE *f; + char line[MAPS_BUF_SZ]; + char *tmp; + + f = fopen("/proc/self/maps", "r"); + if (!f) { + tst_res(TFAIL, "Failed to open /proc/self/maps: %s\n", strerror(errno)); + return -1; + } + + while (1) { + unsigned long start, end, off, ino; + int ret; + + tmp = fgets(line, MAPS_BUF_SZ, f); + if (!tmp) + break; + + buf[0] = '\0'; + ret = sscanf(line, "%lx-%lx %*s %lx %*s %ld %255s", + &start, &end, &off, &ino, + buf); + if ((ret < 4) || (ret > 5)) { + tst_res(TFAIL, "Couldn't parse /proc/self/maps line: %s\n", + line); + fclose(f); + return -1; + } + + if ((start <= addr) && (addr < end)) { + fclose(f); + return 1; + } + } + + fclose(f); + return 0; +}
Test Description: This test tries to allocate hugepages to cover a memory range that straddles the 4GB boundary. Signed-off-by: Geetika <geetika@linux.ibm.com> --- runtest/hugetlb | 1 + testcases/kernel/mem/.gitignore | 1 + .../kernel/mem/hugetlb/hugemmap/hugemmap40.c | 145 ++++++++++++++++++ testcases/kernel/mem/hugetlb/lib/hugetlb.c | 42 +++++ 4 files changed, 189 insertions(+) create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap40.c