[LTP] [PATCH v2 1/5] Hugetlb: Migrating libhugetlbfs counters

[Tarun Sahu]

On Nov 10 2022, Cyril Hrubis wrote:
> Hi!
> > > > +		prev_total = t;
> > > > +		prev_free = f;
> > > > +		prev_resv = r;
> > > > +		prev_surp = s;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	tst_res(TFAIL, "Failure Line %i: Bad %s expected %li, actual %li",
> > >                         ^
> > > 			Never print "Fail/Pass" with tst_res() it's
> > > 			printed based on the flag passed to it.
> > > 
> > > The output would contain Fail and Failed at the same time.
> > > 
Ok Will update it.
> > This doesn't say failed.
> > It says failure-line from which the failure originated.
> > like, 
> > hugemmap10.c:63: FAIL: Failure Line 321, Bad HugePages_Free: expected 5, actual 4
> 
> However that is still redundant information, right?
> 
> The meaning of "FAIL: line xyz" and "FAIL: failure line xyz" is the
> same, the second one is just longer. Let's keep the messages short
> and to the point.
> 
> > > I think that instead of the __LINE__ it would make more sense to pass
> > > the test description as a string as we do with test_counters()
> > > 
> > That will require each line inside test_counters to have unique string
> > description for map, touch, unmap, set_nr_hugepages calls, similiary inside
> > for loop. Which will make user hard to find where they have to look for
> > origin of issue, unless they search for string match.
> > 
> > like,
> > 
> > 	/* untouched, private mmap */
> > 	map(SL_TEST, 1, MAP_PRIVATE, "mmap private no touch");
> > 	unmap(SL_TEST, 1, MAP_PRIVATE, "unmap memory mmaped private no touched");
> > 
> > 	/* touched, private mmap */
> > 	map(SL_TEST, 1, MAP_PRIVATE, "mmap private followed by touch");
> > 	
> > 	touch(SL_TEST, 1, MAP_PRIVATE, "touch memory mmaped private");
> > 	unmap(SL_TEST, 1, MAP_PRIVATE, "unmap memory touched mmaped private");
> > 
> > But I agree, a unique description, will give more information on test run
> > logs. 
> > 
> > What do you think?
> 
> Sounds good.
> 
Ok, Will update it with custom msgs.
> > > > +	if (setjmp(buf))
> > > > +		goto cleanup;
> > > 
> > > This is way beyond ugly. I guess that it would be cleaner to actually
> > > return a pass/fail from the test_counters() function and break the for()
> > > loop based on that value instead of this longjmp trickery.
> > > 
> > > Also I do not think that the current code is correct anyway, because we
> > > skip the unmap() call. So I suppose the correct way would be:
> > > 
> > > 
> > > 	res = test_counters("Untouched, shared", base_nr);
> > > 	unmap(SL_SETUP, 1, MAP_SHARED);
> > > 
> > > 	if (res)
> > > 		break;
> > > 
> > 
> > I was thinking same first. But Thought of adding the checks at each line in
> > test_counters(...) and inside for loop, will make the code unclean. Hence,
> > I chose the setjmp/longjmp mechanism. Only drawback is that mapping was not
> > getting cleaned up (unmap), That we can add in per_iteration_cleanup.
> > 
> > What do you think?
> 
> The reason why I do not like the longjmp() is that it obscures the code
> flow. If we have explicit if () and break; it's clear what is happening.
> With setjmp() you have to search the code for corresponding longjmp()
> calls. It's not that bad in this case but I would still stick to
> avoiding longjmp() unless really necessary.
> 
> > > Or eventually we can make the desing better by unmaping any leftover
> > > mappings in the per_iteration_cleanup(). Then we can just do:
> > > 
> > > 	map()
> > > 	if (test_coutners(...)
> > > 		break;
> > > 	unmap()
> > > 
> > map and unmap do also require return checks, as they also perform
> > verify_counter on expected and original counters.
> 
> I guess that we can also put the map() (touch()) test_counters() unamp()
> sequence to a do_test() fuction then call it from the for() loop in
> run_test(). That would make the code a bit cleaner.
> 
Instead, I am thinking of a defining a macro like this, 

#define CHECK_(fun) ({			\
		if (fun) {					\
				break;				\
			}						\
		})

inside test_counters:

do {
	CHECK_(map(...));
	CHECK_(touch(...));
	CHECK_(unmap(...));
} while(0)

inside for loop of run_test:

CHECK_(map(...));
CHECK_(test_counters(...));
CHECK_(unmap(...));

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