Re: [LTP] [PATCH v2 1/3] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Tarun Sahu <tsahu@linux.ibm.com>]

Hello,
> 
> Tarun Sahu <tsahu@linux.ibm.com> writes:
> 
> > > > +	snprintf(hfile, sizeof(hfile), "%s/ltp_hugetlbfile%d",
> > > > Hopt,
> > > > getpid());
> > > > +	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:")*1024;
> > > > +
> > > > +	fd = SAFE_OPEN(hfile, O_RDWR | O_CREAT, 0600);
> > > > +	SAFE_UNLINK(hfile);
> > > 
> > > I'm guessing opening this file and using it with mmap is a very
> > > common
> > > pattern. If so, it should be encapsulated in tst_hugepage.c.
> > > 
> > Yeah I agree. But the change in tst_hugepage.c will require change
> > in
> > prexisting hugetlb tests.
> 
> You don't need to update existing LTP tests if you add a new flag to
> .tst_test I guess.
yes.
I will add new tst_test flag option for Mount and open file setup 
in next version.
> 
> > > > +}
> > > > +
> > > > +static void cleanup(void)
> > > > +{
> > > > +	if (fd >= 0)
> > > > +		SAFE_CLOSE(fd);
> > > > +	SAFE_UMOUNT(Hopt);
> > > > +}
> > > > +
> > > > +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"},
> > > 
> > > This is a source of confusion. This description suggests we pass
> > > in
> > > an
> > > existing hugetlb mount. However it's actually where it will be
> > > mounted.
> > > 
> > > Perhaps instead we could use set/getmntent to find an existing
> > > hugetlb
> > > mount?  Then if it is not there, try mounting it. This is what we
> > > do
> > > for
> > > CGroups.
> > > 
> > > I'm not sure what difference it makes where the FS is mounted
> > > anyway. Why is it even an option?
> > 
> > Not sure, If user is ok for using premounted fs without their
> > permission. So instead of searching, it will mount where user will
> > explicitly asked for. Or otherwise if not provided, it will mount
> > at
> > temp location under /tmp.
> 
> Does it actually create a new FS or is it just remounting the
> existing
> hugetlb interface in a new place?
> 
> Also it requires privileges to mount an FS. It seems unlikely that
> some
> database wanting to use hugepages would mount it themselves.
> 
> From the kernel docs:
> 
> "If the user applications are going to request huge pages using mmap
> system
> call, then it is required that system administrator mount a file
> system of
> type hugetlbfs"

I agree.
But this will make Hopt option compulsory for user to provide.
and during batch running of tests (/opt/ltp/runltp -f hugetlb), user
will have to modify each test line in /opt/ltp/runtest to provide the
particular mounted option to get the tests running.

Instead, I propose, to follow the same mechanism as the older hugetlb
test follow, create a random temp location under /tmp (/tmp/xxxxxx),
and mount and umount hugetlbfs as needed. For this way, I will also
remove the unnecessary Hopt option. Though need_root will become 
pre-requisite to run the test.

> 
> > I had taken this option from hugemmap01 test. Thinking, it might be
> > to provide user the flexibility incase user doesnt want test to
> > mount
> > fs at random location by default.
> > 
> > I will change the description to "Provide the location to mount the
> > hugetlbfs or default '/tmp/xxxxxxx'"
> > 
> > > > +		{"s:", &nr_opt, "Set the number of the been
> > > > allocated
> > > > hugepages"},
> > > > +		{}
> > > > +	},
> > > > +	.setup = setup,
> > > > +	.cleanup = cleanup,
> > > > +	.test_all = run_test,
> > > > +	.hugepages = {1, TST_NEEDS},
> > > 
> > > When we set this tst_hugepages.c could fill Hopt (which should be
> > > something like tst_hugepages_mount) with the location of the
> > > mount
> > > point. It could also open the hfile fd and store it in a static
> > > variable like tst_hugepage_fd.
> > Yeah, It will move the repeated actions to single location.
> > 
> > This will
> > require change in lib/tst_hugepage.c and which will require
> > change in already existing test under hugetlb/ . Which will be like
> > a
> > new separate change patch series.
> 
> If it results in too many complicated changes, then converting all
> the
> existing tests could be defered to a later time by creating new
> variables.
Ok.
> 


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