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

Shirisha ganta
2024-03-21T06:50:06Z

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