Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Li Wang <liwang@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6590 bytes --]

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},
};



>
>         /*
>          * 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.



> +
>  /*
>   * 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");
        }


        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

[-- Attachment #1.2: Type: text/html, Size: 10431 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp