From: Tarun Sahu <tsahu@linux.ibm.com>
To: Cyril Hrubis <chrubis@suse.cz>
Cc: ltp@lists.linux.it, linux-kselftest@vger.kernel.org,
linux-mm@kvack.org, aneesh.kumar@linux.ibm.com,
linux-kernel@vger.kernel.org
Subject: Re: [LTP] [RFC PATCH] Hugetlb: Migrating hugetlb tests from libhugetlbfs
Date: Mon, 12 Sep 2022 14:03:10 +0530 [thread overview]
Message-ID: <2412537e2e07ebf62fe95971a3022336cde9833a.camel@linux.ibm.com> (raw)
In-Reply-To: <YxsCJi8O+HmMpefq@yuki>
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
next prev parent reply other threads:[~2022-09-12 8:33 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 17:39 [RFC PATCH] Hugetlb: Migrating hugetlb tests from libhugetlbfs Tarun Sahu
2022-09-09 9:06 ` [LTP] " Cyril Hrubis
2022-09-12 8:33 ` Tarun Sahu [this message]
2022-09-12 8:43 ` Cyril Hrubis
2022-09-12 10:06 ` Tarun Sahu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2412537e2e07ebf62fe95971a3022336cde9833a.camel@linux.ibm.com \
--to=tsahu@linux.ibm.com \
--cc=aneesh.kumar@linux.ibm.com \
--cc=chrubis@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=ltp@lists.linux.it \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).