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

[Cyril Hrubis <chrubis@suse.cz>]

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

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

-- 
Cyril Hrubis
chrubis@suse.cz

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