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

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

Hi Richard, 
Thanks for reviewing the patch.

-- skip
> > +static int arch_has_slice_support(void)
> 
> This only appears to be used on __powerpc64__ in next_chunk. So it
> can
> be defined in the ifdef for next_chunk.
> 
> Otherwise we could get unused function warnings and IMO it is more
> readable.

Yeah, I will update it in next version.

> 
> > +{
> > +#ifdef __powerpc64__
> > +	char mmu_type[16];
> > +	FILE *fp;
> > +	int ret;
> > +
> > +	fp = SAFE_POPEN("cat /proc/cpuinfo | grep MMU | awk '{ print
> > $3}'", "r");
> > +	ret = fscanf(fp, "%s", mmu_type);
> > +	pclose(fp);
> > +
> > +	if (ret < 0)
> > +		tst_brk(TBROK, "Failed to determine MMU type");
> > +
> > +	return strcmp(mmu_type, "Hash") == 0;
> > +#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)
> 
> This elif doesn't appear to be applicable.
I missed adding PPC_NO_SEGMENTS cppflags in Makefile
Will update it in next version.

> 
> > +	return 1;
> > +#else
> > +	return 0;
> > +#endif
> > +}
> > +
> > +#ifdef __powerpc64__
> > +static void *next_chunk(void *addr)
> > +{
> > +	if (!arch_has_slice_support())
> > +		return PALIGN(addr,
> > SAFE_READ_MEMINFO("Hugepagesize:")*1024);
> 
> In setup we set hpage_size, but then keep reading it.
Yeah, I missed it, Will update it.
> 
> > +
> > +	if ((unsigned long)addr < 0x100000000UL)
> > +		/* 256M segments below 4G */
> > +		return PALIGN(addr, 0x10000000UL);
> > +	/* 1TB segments above */
> > +	return PALIGN(addr, 0x10000000000UL);
> > +}
> > +#elif defined(__powerpc__) && !defined(PPC_NO_SEGMENTS)
> > +static void *next_chunk(void *addr)
> > +{
> > +	return PALIGN(addr, 0x10000000UL);
> > +}
> > +#elif defined(__ia64__)
> > +static void *next_chunk(void *addr)
> > +{
> > +	return PALIGN(addr, 0x8000000000000000UL);
> > +}
> > +#else
> > +static void *next_chunk(void *addr)
> > +{
> > +	return PALIGN(addr, SAFE_READ_MEMINFO("Hugepagesize:")*1024);
> > +}
> > +#endif
> 
> If these functions are used in later tests I guess they should go in
> tst_hugepages.h
No, These functions are only used in this test.

> 
> > +	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.

> > +}
> > +
> > +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.

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.

> 
> Also .taint_check should be added here to check for the type of Ooops
> that are caused by this test. This makes debugging easier if the
> kernel
> doesn't kill the test process or freeze immediately.
Ok, Will update it in next version.

> 
> > +};
> > diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> > b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> > index f75109f3e..1cfeca95a 100644
> > --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> > +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.h
> > @@ -20,6 +20,9 @@
> >  #include "old_tmpdir.h"
> >  #include "mem.h"
> >  
> > +#define ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
> > +#define PALIGN(p, a) ((void *)ALIGN((unsigned long)(p), (a)))
> > +
> >  #define SHM_RD	0400
> >  #define SHM_WR	0200
> >  #define SHM_RW	(SHM_RD|SHM_WR)
> > -- 
> > 2.31.1
> 
> 


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