[LTP] [PATCH] [PATCH] Migrating the libhugetlbfs/testcases/straddle_4GB.c
Petr Vorel
pvorel@suse.cz
Tue Nov 28 12:50:41 CET 2023
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;
> +}
More information about the ltp
mailing list