Re: [PATCH v2] selftests/mm: hugetlb_madv_vs_map: add underflow test

[Mike Rapoport <rppt@kernel.org>]

On Mon, Aug 31, 2026 at 07:38:49PM +0200, Guillaume Morin wrote:
> On 31 Aug 12:01, Mike Rapoport wrote:
> > > +		return;
> > > +	}
> > > +
> > > +	/* Parent */
> > > +	close(pipe_fds[0]);
> > > +
> > > +	/* First unmap, this will close the vma */
> > > +	if (munmap(huge_ptr, mmap_size) != 0) {
> > > +		kill(pid, SIGKILL);
> > > +		ksft_exit_fail_msg("munmap failed");
> > 
> > Won't this leave the child as a zombie?
> 
> It will, for a brief amount of time then re-parented to init which will
> wait(). Happy to add a waitpid() here but I was concerned it added more
> complexity for not much benefit (it's very very unlikely munmap will
> fail). Let me know.

Letting init reap the child is fine, but there are other issues with
killing the child without wait().

First, kill() deliverers the signal asynchronously, it may happen that the
parent exits before the child and calls _atexit() cleaunps that should restore
nr_hugepages. But since the child still holds the page the restore won't
work.

Right now this is somewhat neutralised because the child returns to main
and calls again for cleanup, but that in turn produces spurious output:

# Planned tests != run tests (3 != 1)
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0

The child should call _exit() to avoid going through ksft_finished() and
hugetlb restore callbacks.

And since you already have the pipe to synchronize the processes, maybe use
that pipe instead of kill?

You can also consider using goto err_cleanup instead of doing cleanup
inside if (err)
 
> > > +	}
> > > +	close(pipe_fds[1]);
> > > +	if (waitpid(pid, NULL, 0) <= 0)
> > > +		ksft_exit_fail_msg("write failed");

waitpid failed?

> > > +
> > > +	nr_reserved = hugetlb_nr_resv_pages(default_huge_page_size());
> > > +	if (nr_reserved == 0) {
> > > +		ksft_test_result_pass("After the child dies, HugePages_Rsvd is properly set to 0\n");
> > 
> > This makes this count the test as two, right? 
> 
> Yes, it checks both that there is no underflow but that the count is
> restorred as well. is that a problem? 
> 
> > And do you need to re-check what happens after the child exits if you
> > already detected the underflow?
> 
> Need is a strong word. It can be done either way really.
> 
> > 
> > > +	} else {
> > > +		ksft_exit_fail_msg("Unexpected HugePages_Rsvd=%ld after the child termination munmap, should be 0 or -1.  Repeat the test\n",
> > > +				   nr_reserved);
> > > +	}
> > > +}
> > > +
> > > +int main(void)
> > > +{
> > > +	ksft_print_header();
> > > +	ksft_set_plan(3);
> > 
> > The comment at the header implies we've added the second test, so they plan
> > should be bumped to 2.
> 
> So you're saying that every test function should only fail or pass once?

Not necessarily, but comment saying "setup a second test" and the
test_underflow() name imply that this is one test rather than two.
 
> -- 
> Guillaume Morin <guillaume@morinfr.org>
> 

-- 
Sincerely yours,
Mike.