[LTP] [RFC PATCH] Hugetlb: Migrating hugetlb tests from libhugetlbfs
Tarun Sahu
tsahu@linux.ibm.com
Mon Sep 12 10:33:10 CEST 2022
Hi Cyril, Thanks for reviewing the patch.
Below I have added my
comments. Will update changes in V2.
On Fri, 2022-09-09 at 11:06 +0200,
Cyril Hrubis wrote:
> Hi!
> > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> > ---
> > runtest/hugetlb | 2 +
> > testcases/kernel/mem/.gitignore | 1 +
> > .../kernel/mem/hugetlb/hugemmap/hugemmap07.c | 106
> > ++++++++++++++++++
> > 3 files changed, 109 insertions(+)
> > create mode 100644
> > testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> >
> > diff --git a/runtest/hugetlb b/runtest/hugetlb
> > index f719217ab..ee02835d3 100644
> > --- a/runtest/hugetlb
> > +++ b/runtest/hugetlb
> > @@ -3,6 +3,8 @@ hugemmap02 hugemmap02
> > hugemmap04 hugemmap04
> > hugemmap05 hugemmap05
> > hugemmap06 hugemmap06
> > +hugemmap07 hugemmap07
> > +
> > 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 ff2910533..df5256ec8 100644
> > --- a/testcases/kernel/mem/.gitignore
> > +++ b/testcases/kernel/mem/.gitignore
> > @@ -4,6 +4,7 @@
> > /hugetlb/hugemmap/hugemmap04
> > /hugetlb/hugemmap/hugemmap05
> > /hugetlb/hugemmap/hugemmap06
> > +/hugetlb/hugemmap/hugemmap07
> > /hugetlb/hugeshmat/hugeshmat01
> > /hugetlb/hugeshmat/hugeshmat02
> > /hugetlb/hugeshmat/hugeshmat03
> > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > new file mode 100644
> > index 000000000..798735ed0
> > --- /dev/null
> > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * License/Copyright Details
> > + */
>
> There should be a SPDX licence identifier here instead.
>
> Also testcase should include a special ascii-doc formatted comment
> here
> that describes the purpose of the test.
As mentioned in the patch description, there is a conflict in license,
That is why, I have avoided to put any of them in the header. Once
confirmed within the community, I can add the original license here.
(GPL2.1+) as
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
this says only to add code with GPL2.0+.
>
> > +#define _GNU_SOURCE
> > +#include <stdio.h>
> > +#include <sys/mount.h>
> > +#include <limits.h>
> > +#include <sys/param.h>
> > +#include <sys/types.h>
> > +
> > +#include "tst_test.h"
> > +
> > +#define P0 "ffffffff"
> > +#define IOSZ 4096
> > +char buf[IOSZ] __attribute__((aligned(IOSZ)));
> > +static int fildes = -1, nfildes = -1;
> > +static char TEMPFILE[MAXPATHLEN];
> > +static char NTEMPFILE[MAXPATHLEN];
>
> Uppercase is reserved for macros in C.
>
> Have you run 'make check' to check for common mistakes before
> submitting?
>
> > +void test_directio(void)
>
> should be static
Ok Will update in V2.
>
> > +{
> > + long hpage_size;
> > + void *p;
> > + int ret;
> > +
> > + hpage_size = SAFE_READ_MEMINFO("Hugepagesize:");
> > +
> > + fildes = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0600);
> > + nfildes = SAFE_OPEN(NTEMPFILE, O_CREAT|O_EXCL|O_RDWR|O_DIRECT,
> > 0600);
>
> I would say that fd and nfd in the original code was were better
> names,
> shorter and to the point. See also:
>
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
I agree, I tried to follow what was in hugemmap01..06 test cases to
keep things similar all across the tests. I will update it in v2.
>
> > + p = mmap(NULL, hpage_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
> > fildes, 0);
> > + if (p == MAP_FAILED)
> > + tst_brk(TFAIL | TERRNO, "mmap() Failed on %s",
> > TEMPFILE);
>
> We do have SAFE_MMAP() as well.
Yeah, I will update them in v2.
>
> > + memcpy(p, P0, 8);
> > +
> > + /* Direct write from huge page */
> > + ret = write(nfildes, p, IOSZ);
> > + if (ret == -1)
> > + tst_brk(TFAIL | TERRNO, "Direct-IO write from huge
> > page");
> > + if (ret != IOSZ)
> > + tst_brk(TFAIL, "Short direct-IO write from huge page");
> > + if (lseek(nfildes, 0, SEEK_SET) == -1)
> > + tst_brk(TFAIL | TERRNO, "lseek");
> > +
> > + /* Check for accuracy */
> > + ret = read(nfildes, buf, IOSZ);
> > + if (ret == -1)
> > + tst_brk(TFAIL | TERRNO, "Direct-IO read to normal
> > memory");
> > + if (ret != IOSZ)
> > + tst_brk(TFAIL, "Short direct-IO read to normal
> > memory");
> > + if (memcmp(P0, buf, 8))
> > + tst_brk(TFAIL, "Memory mismatch after Direct-IO
> > write");
> > + if (lseek(nfildes, 0, SEEK_SET) == -1)
> > + tst_brk(TFAIL | TERRNO, "lseek");
>
> And we have SAFE_WRITE(), SAFE_READ(), and SAFE_LSEEK() as well.
ok Will update these too in v2.
>
> Also tst_brk(TFAIL, "") usage is deprecated and should not be used in
> new tests.
Ok, Will update it to tst_res.
>
> > + /* Direct read to huge page */
> > + memset(p, 0, IOSZ);
> > + ret = read(nfildes, p, IOSZ);
> > + if (ret == -1)
> > + tst_brk(TFAIL | TERRNO, "Direct-IO read to huge page");
> > + if (ret != IOSZ)
> > + tst_brk(TFAIL, "Short direct-IO read to huge page");
> > +
> > + /* Check for accuracy */
> > + if (memcmp(p, P0, 8))
> > + tst_brk(TFAIL, "Memory mismatch after Direct-IO read");
> > + tst_res(TPASS, "Successfully tested Hugepage Direct I/O");
>
> You should close filedescriptors and unmap memory here, otherwise the
> test will fail with large enough -i parameter.
Ok, I will update it too in v2.
>
> > +}
> > +
> > +void setup(void)
>
> should be static.
Ok Will update it in V2.
>
> > +{
> > + if (tst_hugepages == 0)
> > + tst_brk(TCONF, "Not enough hugepages for testing.");
> > +
> > + if (!Hopt)
> > + Hopt = tst_get_tmpdir();
> > + SAFE_MOUNT("none", Hopt, "hugetlbfs", 0, NULL);
> > +
> > + snprintf(TEMPFILE, sizeof(TEMPFILE), "%s/mmapfile%d", Hopt,
> > getpid());
>
> Ideally all files created outside of the test temporary directory
> should
> be prefixed with "ltp_"
ok, Will update it in V2.
>
> > + snprintf(NTEMPFILE, sizeof(NTEMPFILE), "%s/nmmapfile%d",
> > "/home/", getpid());
>
> Please do not create any files outside of the test temporary
> directory,
> also as the temporary directory is unique already, there is no need
> to
> actually create the second tempfile name like this. All we need to do
> is
> to is something as:
>
> #define NTEMPFILE "tempfile"
>
Ok Will update it in V2.
> > +}
> > +
> > +void cleanup(void)
> > +{
> > + close(fildes);
> > + close(nfildes);
> > + remove(TEMPFILE);
> > + remove(NTEMPFILE);
> > + umount2(Hopt, MNT_DETACH);
> > +}
> > +
> > +static struct tst_test test = {
> > + .needs_root = 1,
> > + .needs_tmpdir = 1,
> > + .options = (struct tst_option[]) {
> > + {"H:", &Hopt, "Location of hugetlbfs, i.e. -H
> > /var/hugetlbfs"},
> > + {"s:", &nr_opt, "Set the number of the been allocated
> > hugepages"},
> > + {}
> > + },
> > + .setup = setup,
> > + .cleanup = cleanup,
> > + .test_all = test_directio,
> > + .hugepages = {2, TST_REQUEST},
> > +};
> > --
> > 2.31.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
More information about the ltp
mailing list