[LTP] [PATCH v4 2/7] Hugetlb: Migrating libhugetlbfs counters

[Tarun Sahu]

Hi, 
Thanks for reviewing the patch, Cyril.
Please find my comments inline.
On Nov 16 2022, Cyril Hrubis wrote:
> Hi!
> > +#define CHECK_(fun)	\
> > +		{			\
> > +		if (fun)	\
> > +			break;	\
> > +	}
> 
> This is working around the tooling we have I would rather have a look
> into the checkpatch script to see if we could tweak the rule rather than
> to introduce this messy code.
> 
> Just submit the patch with sane code and I will check for what I can do
> with the script to get rid of the error.
Ok.

--Skip
> > +static long hpage_size;
> > +
> > +	if (t != et) {
> > +		tst_res(TFAIL, "At Line %i:While %s: Bad "MEMINFO_HPAGE_TOTAL
> > +				" expected %li, actual %li", line, desc, et, t);
> 
> We do have tst_res_() that can be called as:
> 
> 	tst_res_(__FILE__, line,
>                  "%s bad " MEMINFO_HPAGE_TOTAL " = %li expected %li",
> 		 desc, et, t);
Ok. Will update it.
> 
> > +out:
> > +	return verify_counters(line, desc, et, ef, er, es);
> > +}
> > +#define set_nr_hugepages(c, d) CHECK_(set_nr_hugepages_(c, d, __LINE__))
> 
> The macro name should be uppercase so that it's clear that it's a macro
> and not a simple function. With that we can also drop the underscore
> from the actual function name too.
> 
I avoided it deliberately knowing that these macros are being called too
often, keeping them in uppercase would have made the code look too messy.
Then, I looked that there was tst_res and similiar macros too which were
kept in smallcase too (which is wrapper to tst_res_ function), That is why
I kept them smallcase.

--skip
> > +static int test_counters_(char *desc, int base_nr)
> > +{
> > +	int fail = 1;
> > +
> > +	tst_res(TINFO, "%s...", desc);
> > +
> > +	do {
> > +		set_nr_hugepages(base_nr, "initializing hugepages pool");
> > +
> > +		/* untouched, shared mmap */
> > +		map(SL_TEST, 1, MAP_SHARED, "doing mmap shared with no touch");
> > +		unmap(SL_TEST, 1, MAP_SHARED, "doing munmap on shared with no touch");
> > +
> > +		/* untouched, private mmap */
> > +		map(SL_TEST, 1, MAP_PRIVATE, "doing mmap private with no touch");
> > +		unmap(SL_TEST, 1, MAP_PRIVATE, "doing munmap private with on touch");
> > +
> > +		/* touched, shared mmap */
> > +		map(SL_TEST, 1, MAP_SHARED, "doing mmap shared followed by touch");
> > +		touch(SL_TEST, 1, MAP_SHARED, "touching the addr after mmap shared");
> > +		unmap(SL_TEST, 1, MAP_SHARED, "doing munmap shared after touch");
> > +
> > +		/* touched, private mmap */
> > +		map(SL_TEST, 1, MAP_PRIVATE, "doing mmap private followed by touch");
> > +		touch(SL_TEST, 1, MAP_PRIVATE, "touching the addr after mmap private");
> > +		unmap(SL_TEST, 1, MAP_PRIVATE, "doing munmap private after touch");
> > +
> > +		/* Explicit resizing during outstanding surplus */
> > +		/* Consume surplus when growing pool */
> > +		map(SL_TEST, 2, MAP_SHARED, "doing mmap to consume surplus");
> > +		set_nr_hugepages(MAX(base_nr, 1), "setting hugepages pool to consume surplus");
> > +
> > +		/* Add pages once surplus is consumed */
> > +		set_nr_hugepages(MAX(base_nr, 3), "Adding more pages after consuming surplus");
> > +
> > +		/* Release free huge pages first */
> > +		set_nr_hugepages(MAX(base_nr, 2), "Releasing free huge pages");
> > +
> > +		/* When shrinking beyond committed level, increase surplus */
> > +		set_nr_hugepages(base_nr, "increasing surplus counts");
> > +
> > +		/* Upon releasing the reservation, reduce surplus counts */
> > +		unmap(SL_TEST, 2, MAP_SHARED, "reducing surplus counts");
> > +		fail = 0;
> > +	} while (0);
> > +
> > +	if (fail)
> > +		return -1;
> > +	tst_res(TINFO, "OK");
> > +	return 0;
> 
> Maybe it would be a bit nicer to have actually two different macros so
> that we don't have to resort to this do {} while (0) trickery e.g.
> 
> #define CHECK_BREAK(cond) if (cond) break;
> #define CHECK_RETURN(cond) if (cond) return -1;
> 
> #define MAP_BREAK(s, h, f, d) CHECK_BREAK(map(s, h, f, d, __LINE__))
> #define MAP_RETURN(s, h, f, d) CHECK_RETURN(map(s, h, f, d, __LINE__))
> 
> Then the check counters would look much better.
> 
> Or we can just stick to the RETURN variants and put the body of the loop
> in the runtest() function into do_interation() function.
> 
I like the second idea, as it will only have one macro. which will keep the
code less messy. But anyway, I had tried this already, if (cond) return; is
not allowed. "make check" throws warning saying, "Macros with flow control
statements should be avoided". Only way I could see is to use break.
> > +
> > -- 
> > 2.31.1
> > 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp