[LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support
Tarun Sahu
tsahu@linux.ibm.com
Mon Oct 31 12:08:54 CET 2022
Hello,
Thanks for reviewing patch Li,
On Oct 31 2022, Li Wang wrote:
> Hi Tarun,
>
> This version is much better, comments are inline below.
>
> On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:
>
> > Most of libhugetlbfs test require mounted hugetlbfs and random opened
> > unlinked file for follow-up test actions.
> >
> > Here, this patch adds two new field in tst_test struct(include/tst_test.h)
> > which user can set if the test requires mounted hugetlbfs and other one
> > for if test requires opened unlinked file.
> >
> > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> > ---
> > include/tst_test.h | 20 +++++++++++++++++-
> > lib/tst_test.c | 51 +++++++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/tst_test.h b/include/tst_test.h
> > index a69965b95..f36382ae9 100644
> > --- a/include/tst_test.h
> > +++ b/include/tst_test.h
> > @@ -131,7 +131,7 @@ struct tst_tag {
> > };
> >
> > extern unsigned int tst_variant;
> > -
> > +extern int tst_hugetlb_fd;
> > #define TST_NO_HUGEPAGES ((unsigned long)-1)
> >
> > #define TST_UNLIMITED_RUNTIME (-1)
> > @@ -176,6 +176,18 @@ struct tst_test {
> > int all_filesystems:1;
> > int skip_in_lockdown:1;
> > int skip_in_compat:1;
> > + /*
> > + * If set, the test function will create a hugetlbfs mount point
> > + * at /tmp/xxxxxx, where xxxxxx is a random string.
> > + */
> > + int needs_hugetlbfs:1;
> > + /*
> > + * If set, the test function will create and open a random file
> > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs
> > must
> > + * be set. The file descriptior will be set in tst_hugetlb_fd.
> > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup).
> > + */
> > + int needs_unlinked_hugetlb_file:1;
> >
>
> Why not consider encapsulating these two new fields in 'struct
> tst_hugepage' ?
>
> Then the tst_test in the case can simply initialize to:
>
> ....
> static struct tst_test test = {
> .needs_root = 1,
> .taint_check = TST_TAINT_D | TST_TAINT_W,
> .setup = setup,
> .test_all = run_test,
> .hugepages = {1, TST_NEEDS, 1, 1},
> };
>
Ok, I will move these fields in tst_hugepages struct.
>
>
> >
> > /*
> > * The skip_filesystems is a NULL terminated list of filesystems
> > the
> > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void);
> > */
> > void tst_set_max_runtime(int max_runtime);
> >
> > +/*
> > + * Create and open a random file inside the given dir path.
> > + * It unlinks the file after opening and return file descriptor.
> > + */
> > +int tst_create_unlinked_file(const char *path);
> >
>
> what about renaming this function to tst_'get|create'_unlinked_file_fd?
> I guess the 'fd' part should be emphasized here.
>
OK. Will update it.
>
>
> > +
> > /*
> > * Returns path to the test temporary directory in a newly allocated
> > buffer.
> > */
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 8ccde1629..43cba1004 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -925,7 +925,8 @@ static int needs_tmpdir(void)
> > tst_test->needs_device ||
> > tst_test->mntpoint ||
> > tst_test->resource_files ||
> > - tst_test->needs_checkpoints;
> > + tst_test->needs_checkpoints ||
> > + tst_test->needs_hugetlbfs;
> > }
> >
> > static void copy_resources(void)
> > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char
> > *mntpoint)
> > }
> > }
> >
> > +static void prepare_and_mount_hugetlb_fs(void)
> > +{
> > + tst_test->mntpoint = tst_get_tmpdir();
> > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL);
> > + mntpoint_mounted = 1;
> > +}
> > +
> > +int tst_create_unlinked_file(const char *path)
> > +{
> > + char template[PATH_MAX];
> > + int fd;
> > +
> > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> > + path, TCID);
> > +
> > + fd = mkstemp(template);
> > + if (fd < 0)
> > + tst_brk(TBROK | TERRNO,
> > + "%s: mkstemp(%s) failed", __func__, template);
> > +
> > + SAFE_UNLINK(template);
> > + return fd;
> > +}
> > +
> > static const char *limit_tmpfs_mount_size(const char *mnt_data,
> > char *buf, size_t buf_size, const char *fs_type)
> > {
> > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void)
> > tst_cg_init();
> > }
> >
> > +int tst_hugetlb_fd = -1;
> > +
> > static void do_setup(int argc, char *argv[])
> > {
> > if (!tst_test)
> > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[])
> > }
> > }
> >
> > + if (tst_test->needs_hugetlbfs)
> > + prepare_and_mount_hugetlb_fs();
> > +
> > + if (tst_test->needs_unlinked_hugetlb_file) {
> > + if (!(tst_test->needs_hugetlbfs)) {
> > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file
> > "
> > + "requires option needs_hugetlbfs");
> > + }
> > + tst_hugetlb_fd =
> > tst_create_unlinked_file(tst_test->mntpoint);
> > + }
> > +
> >
>
> Seems we have to add a confliction check[1] to avoid multiple mounting
> at 'tst_test->mntpoint'. Or maybe go another method to move all the
> hugetlbfs
> operations into tst_hugetlb.c to isolate details from the tst_test library,
> but
> this will require more changes for all preexisting hugetlb tests.
>
>
> [1] something like this:
>
> @@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[])
> }
>
> if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
> - !!tst_test->needs_device > 1) {
> + !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) {
> tst_brk(TBROK,
> - "Two or more of needs_{rofs, devfs, device} are
> set");
> + "Two or more of needs_{rofs, devfs, device,
> hugetlb} are set");
> }
>
OK. Will update it.
>
> if (tst_test->needs_device && !mntpoint_mounted) {
> > tdev.dev = tst_acquire_device_(NULL,
> > tst_test->dev_min_size);
>
>
> > @@ -1299,8 +1337,15 @@ static void do_cleanup(void)
> > if (ovl_mounted)
> > SAFE_UMOUNT(OVL_MNT);
> >
> > - if (mntpoint_mounted)
> > - tst_umount(tst_test->mntpoint);
> > + if (tst_hugetlb_fd >= 0)
> > + SAFE_CLOSE(tst_hugetlb_fd);
> > +
> > + if (mntpoint_mounted) {
> > + if (tst_test->needs_hugetlbfs)
> > + SAFE_UMOUNT(tst_test->mntpoint);
> > + else
> > + tst_umount(tst_test->mntpoint);
> > + }
> >
> > if (tst_test->needs_device && tdev.dev)
> > tst_release_device(tdev.dev);
> > --
> > 2.31.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
> >
>
> --
> Regards,
> Li Wang
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
More information about the ltp
mailing list