Re: [LTP] [PATCH v3] Migrating the libhugetlbfs/testcases/truncate.c test

Petr Vorel
2023-11-28T11:11:39Z

Hi,

Maybe I'm slow, but it was not not obvious from the subject "Migrating the
libhugetlbfs/testcases/truncate.c test" that you migrate test from:
https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/truncate.c

I'd personally have subject

Add hugemmap33 (very sort description of the test)

And then later mention in the commit message that the test originates from
https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/truncate.c

NOTE: if you run test more times (-iN), it fails, e.g.:

./hugemmap33 -i2
tst_hugepage.c:84: TINFO: 1 hugepage(s) reserved
tst_test.c:1037: TINFO: Mounting none to /tmp/LTP_hugiULTJ7/hugetlbfs fstyp=hugetlbfs flags=0
tst_test.c:1690: TINFO: LTP version: 20230929-152-gce87c8de3
tst_test.c:1574: TINFO: Timeout per run is 0h 00m 30s
hugemmap33.c:56: TPASS: Expected SIGBUS triggered
tst_test.c:1634: TBROK: Test killed by SIGBUS!

Could you please fix it?

Also, we use static whenever we can, please fix these:

$ make check-hugemmap33
CHECK testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
hugemmap33.c:61:6: warning: Symbol 'setup' has no prototype or library ('tst_') prefix. Should it be static?
hugemmap33.c:71:6: warning: Symbol 'cleanup' has no prototype or library ('tst_') prefix. Should it be static?

> Test Description:
This line is useless.

> Test is used to verify the correct functionality and
> compatibility of the library with the "truncate" system
> call when operating on files residing in a mounted
> huge page filesystem.

> Signed-off-by: Shirisha G <shirisha@linux.ibm.com>
> ---
> v3:
> -Addressed the below requested changes
> 1. Removed RANDOM_CONSTANT
> 2. Made hpage_size and fd to static
> 3. Used a volatile variable as a flag
> to pass test in the run_test()
> 4. Removed the failure condition for SAFE_MMAP()
> 5. Have setup the handler in the setup()
> 6. Added SAFE_MUNMAP()
> 7. Ran make check and fixed all issues
> ---
> v2:
> -Corrected typo
> ---
> runtest/hugetlb | 1 +
> testcases/kernel/mem/.gitignore | 1 +
> .../kernel/mem/hugetlb/hugemmap/hugemmap33.c | 88 +++++++++++++++++++
> 3 files changed, 90 insertions(+)
> create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c

> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 299c07ac9..1300e80fb 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -35,6 +35,7 @@ hugemmap29 hugemmap29
> hugemmap30 hugemmap30
> hugemmap31 hugemmap31
> hugemmap32 hugemmap32
> +hugemmap33 hugemmap33
> 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..d130d4dcd 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/hugemmap33
> /hugetlb/hugeshmat/hugeshmat01
> /hugetlb/hugeshmat/hugeshmat02
> /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> new file mode 100644
> index 000000000..3405627f6
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap33.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2005-2006 IBM Corporation.
Maybe also your or LTP copyright here, as clearly there is LTP specific code.

> + * Author: David Gibson & Adam Litke
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Test Name: truncate
This is wrong and useless, please remove it.
> + *
> + * Test case is used to verify the correct functionality
> + * and compatibility of the library with the "truncate" system call when
> + * operating on files residing in a mounted huge page filesystem.
> + */
> +
> +#include "hugetlb.h"
> +#include <setjmp.h>
> +#include <signal.h>
> +
> +#define MNTPOINT "hugetlbfs/"
> +
> +static long hpage_size;
> +static int fd;
> +static sigjmp_buf sig_escape;
> +static volatile int test_pass;
> +static int sigbus_count;
> +
> +static void sigbus_handler(int signum)

hugemmap33.c:29:32: warning: unused parameter ‘signum’ [-Wunused-parameter]
29 | static void sigbus_handler(int signum)

Therefore use
static void sigbus_handler(int signum LTP_ATTRIBUTE_UNUSED)

> +{
> + test_pass = 1;
> + siglongjmp(sig_escape, 17);
> +}
> +
> +static void run_test(void)
> +{
> + void *p;
> + volatile unsigned int *q;
> +
> + sigbus_count = 0;
> + test_pass = 0;
> + int err;
> +
> + p = SAFE_MMAP(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_SHARED,
> + fd, 0);
> + q = p;
> + *q = 0;
> + err = ftruncate(fd, 0);
Blank line here would help readability.

> + if (err)
> + tst_res(TFAIL, "ftruncate failed");
Also here.
> + if (sigsetjmp(sig_escape, 1) == 0)
> + *q;
> + else
> + sigbus_count++;
And here.
> + if (sigbus_count != 1)
> + tst_res(TFAIL, "Didn't SIGBUS");
And here.
> + if (test_pass == 1)
> + tst_res(TPASS, "Expected SIGBUS triggered");
And here.
> + SAFE_MUNMAP(p, hpage_size);
> +}