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

Tarun Sahu
2022-11-16T16:27:01Z

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