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@xxxxxxxxxxxxx> > > --- > > 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
