Message-ID: <2412537e2e07ebf62fe95971a3022336cde9833a.camel@linux.ibm.com>
Date:   Mon, 12 Sep 2022 14:03:10 +0530
From:   Tarun Sahu <tsahu@...ux.ibm.com>
To:     Cyril Hrubis <chrubis@...e.cz>
Cc:     ltp@...ts.linux.it, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, aneesh.kumar@...ux.ibm.com,
        linux-kernel@...r.kernel.org
Subject: Re: [LTP] [RFC PATCH] Hugetlb: Migrating hugetlb tests from
 libhugetlbfs

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@...ux.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

Powered by blists - more mailing lists