Re: [PATCH v2 13/53] selftests/mm: khugepaged: use ksefltest framework

[Mike Rapoport]

Hi Sarthak,

(please trim your responses next time)

On Tue, Apr 28, 2026 at 06:35:51PM +0530, Sarthak Sharma wrote:
> On 4/18/26 4:24 PM, Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <rppt@xxxxxxxxxx>
> > 
> > Convert khugepaged tests to use kselftest framework for reporting and
> > tracking successful and failing runs.

...

> > @@ -830,16 +783,12 @@ static void collapse_compound_extreme(struct collapse_context *c, struct mem_ops
> >  	int i;
> >  
> >  	p = ops->setup_area(1);
> > +	ksft_print_msg("Construct PTE page table full of different PTE-mapped compound pagesd\n");
> 
> A small typo here, "pagesd" should be "pages".

Thanks.
 
> >  	for (i = 0; i < hpage_pmd_nr; i++) {
> > -		printf("\rConstruct PTE page table full of different PTE-mapped compound pages %3d/%d...",
> > -				i + 1, hpage_pmd_nr);
> > -
> >  		madvise(BASE_ADDR, hpage_pmd_size, MADV_HUGEPAGE);
> >  		ops->fault(BASE_ADDR, 0, hpage_pmd_size);
> > -		if (!ops->check_huge(BASE_ADDR, 1)) {
> > -			printf("Failed to allocate huge page\n");
> > -			exit(EXIT_FAILURE);
> > -		}
> > +		if (!ops->check_huge(BASE_ADDR, 1))
> > +			ksft_exit_fail_msg("Failed to allocate huge page\n");
> 
> In case we are not able to allocate a hugepage at fault, should this be
> reported as a per-test failure instead of bailing out the whole binary
> with ksft_exit_fail_msg()? In alloc_at_fault(), a failure to allocate a
> hugepage is reported as a normal test result via
> ksft_test_result_report(), so should this case behave similarly or is it
> intentionally treated as fatal? I realize it was being treated as fatal
> before this patch as well, but just wanted to check whether that is
> necessary.

I don't think it's necessary, but let's address it separately. Here I only
mechanically convert the code to use kselftest helpers.
 
> Also, when ksft_exit_fail_msg() is called, totals are printed twice:
> once from ksft_exit_fail_msg() itself, and then again when exit() runs
> the restore_settings_atexit handler.

This is temporal, restore_settings_atexit() is removed from khugepaged in
later patches. If anybody feels strongly about it I can fix, but IMO it's
not really important.
 
> > @@ -1242,8 +1186,6 @@ int main(int argc, char **argv)
> >  	save_settings();
> >  	thp_push_settings(&default_settings);
> 
> Also, can save_settings() and thp_push_settings() be moved below
> ksft_set_plan(), preserving their order?

Yes, you are welcome to send a patch ;-)
There a lot of possible improvements all over the place and I wanted to
keep changes here to minimum. Let's keep it for the next round.

> Since save_settings() is printing a debug line of its own, it would look
> better if ksft plan is printed just below the TAP version line.

The message from save_settings() is printed after the TAP header:

# TAP version 13
# # Save THP and khugepaged settings... OK
# 1..26

-- 
Sincerely yours,
Mike.