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

Tarun Sahu
2022-10-31T18:03:02Z

Hi Cyril,
Please find my comments inline.

On Oct 31 2022, Cyril Hrubis wrote:
> Hi!
> > 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},
> > };
>
> I do not like that we have magic constants in the .hugepages that are
> not self describing. I would treat the hugetltbfs just as we treat
> devfs, that would be:
>
> #define MNTPOINT "hugetlbfs/"
> #define HUGEFILE MNTPOINT "hugefile"
>
> static int huge_fd;
>
> static void setup(void)
> {
> huge_fd = tst_creat_unlinked(HUGEFILE);
> ...
> }
>
> static void cleanup(void)
> {
> if (huge_fd > 0)
> SAFE_CLOSE(huge_fd);
> }
>
> static struct tst_test test = {
> ...
> .mntpoint = MNTPOINT,
> .needs_hugetlbfs = 1,
> .setup = setup,
> .cleanup = cleanup,
> ...
> }
>
>
> What do you think?
>
My original idea behind putting it in tst_test struct instead of
tst_hugepages was based on below two reasoning-

1. tst_hugepages seems to have only hugepages related info. It would be
better to rename it to tst_hugetlb and rename <hugepages> field in tst_test
struct to <hugetlb>. If we rename it which will require changes in already
existing tests. and Moreover, there were similar field like needs_tmpdir
in tst_test so I put it(needs_unlinked_hugetlb_file) in tst_test.
2. There was already related fields in tst_test for mounting fs, like
needs_devfs, needs_rofs, So keeping all the needs_ABCfs at same structure.

> --
> Cyril Hrubis
> chrubis@suse.cz
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp