| Message ID | 20230929091401.205277-1-shirisha@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] Migrating the libhugetlbfs/testcases/truncate.c test | expand |
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);
> +}
Kind regards,
Petr
Hi, ... > +static void sigbus_handler(int signum) > +{ > + test_pass = 1; > + siglongjmp(sig_escape, 17); What 17 stands for? Is there any constant which could be used? Or can you define something (the name of the constant would be self-describing). > +} > + > +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); > + if (err) > + tst_res(TFAIL, "ftruncate failed"); We have SAFE_FTRUNCATE(), error check is not needed. Kind regards, Petr
On Tue, 2023-11-28 at 12:10 +0100, Petr Vorel wrote: > 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? Will take care of this in V4 > > 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. This is the general practice followed by all the test cases in this folder and I followed the same. > > > 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. Will take care of this in V4 > > + * > > + * 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) Will take care of this in V4 > > > +{ > > + 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. Will take care of this in V4 > > > + if (err) > > + tst_res(TFAIL, "ftruncate failed"); > Also here. Will take care of this in V4 > > + if (sigsetjmp(sig_escape, 1) == 0) > > + *q; > > + else > > + sigbus_count++; > And here. Will take care of this in V4 > > + if (sigbus_count != 1) > > + tst_res(TFAIL, "Didn't SIGBUS"); > And here. Will take care of this in V4 > > + if (test_pass == 1) > > + tst_res(TPASS, "Expected SIGBUS triggered"); > And here. Will take care of this in V4 > > + SAFE_MUNMAP(p, hpage_size); > > +} > > Kind regards, > Petr
On Tue, 2023-11-28 at 12:22 +0100, Petr Vorel wrote: > Hi, > > ... > > +static void sigbus_handler(int signum) > > +{ > > + test_pass = 1; > > + siglongjmp(sig_escape, 17); > What 17 stands for? Is there any constant which could be used? Or can > you define > something (the name of the constant would be self-describing). The value 17 is often chosen as a non-zero return value for sigsetjmp because it is unlikely to be a valid return value from a normal function. By using a non-zero value, you can distinguish between a direct return from sigsetjmp (where the return value is 0) and a return after a siglongjmp (where the return value is non-zero). By using a non-zero value (like 17), you can differentiate between normal function returns and jumps due to exceptional conditions. he specific value chosen (such as 17) can vary based on the programmer’s preference or the context of the application. Some developers might use other non-zero values, but 17 has become a convention due to its uniqueness and readability. > > +} > > + > > +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); > > + if (err) > > + tst_res(TFAIL, "ftruncate failed"); > We have SAFE_FTRUNCATE(), error check is not needed. Will take care of this in V4 > > Kind regards, > Petr
> > ... > > > +static void sigbus_handler(int signum) > > > +{ > > > + test_pass = 1; > > > + siglongjmp(sig_escape, 17); > > What 17 stands for? Is there any constant which could be used? Or can > > you define > > something (the name of the constant would be self-describing). > The value 17 is often chosen as a non-zero return value for sigsetjmp > because it is unlikely to be a valid return value from a normal > function. By using a non-zero value, you can distinguish between a > direct return from sigsetjmp (where the return value is 0) and a return > after a siglongjmp (where the return value is non-zero). > By using a non-zero value (like 17), you can differentiate between > normal function returns and jumps due to exceptional conditions. he > specific value chosen (such as 17) can vary based on the programmer’s > preference or the context of the application. Some developers might use > other non-zero values, but 17 has become a convention due to its > uniqueness and readability. Thanks for info! Kind regards, Petr
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. + * Author: David Gibson & Adam Litke + */ + +/*\ + * [Description] + * + * Test Name: truncate + * + * 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) +{ + 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); + if (err) + tst_res(TFAIL, "ftruncate failed"); + if (sigsetjmp(sig_escape, 1) == 0) + *q; + else + sigbus_count++; + if (sigbus_count != 1) + tst_res(TFAIL, "Didn't SIGBUS"); + if (test_pass == 1) + tst_res(TPASS, "Expected SIGBUS triggered"); + SAFE_MUNMAP(p, hpage_size); +} + + +void setup(void) +{ + struct sigaction my_sigaction; + + my_sigaction.sa_handler = sigbus_handler; + SAFE_SIGACTION(SIGBUS, &my_sigaction, NULL); + hpage_size = tst_get_hugepage_size(); + fd = tst_creat_unlinked(MNTPOINT, 0); +} + +void cleanup(void) +{ + if (fd > 0) + SAFE_CLOSE(fd); +} + +static struct tst_test test = { + .needs_root = 1, + .mntpoint = MNTPOINT, + .needs_hugetlbfs = 1, + .needs_tmpdir = 1, + .setup = setup, + .cleanup = cleanup, + .test_all = run_test, + .hugepages = {1, TST_NEEDS}, +};
Test Description: 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