Re: [PATCH v5] selftests/mm: add process_madvise() tests

[wang lian <lianux.mm@gmail.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2952 bytes --]

> On Tue, Jul 15, 2025 at 06:58:05PM +0800, wang lian wrote:
> > > On Mon, Jul 14, 2025 at 08:25:33PM +0800, wang lian wrote:

> > > > +	/* Cleanup */
> > > > +	kill(self->child_pid, SIGKILL);
> > > > +	waitpid(self->child_pid, NULL, 0);
> > > > +	if (pidfd >= 0)
> > > > +		close(pidfd);

> > > The cleanup here won't get run if we skip or assert, skipping will
> > > return immediately (you could replace the return with a 'goto cleanup')
> > > and the asserts will exit the test immediately.  This will mean we leak

> > Fortunately, this situation is handled by FIXTURE_TEARDOWN_PARENT, 
> > which reliably takes care of cleanup when the test exits early via ASSERT_* or SKIP(). 

> > During earlier testing (in v3), I did run into an issue where a missing cleanup 
> > led to run_vmtests hanging,which prompted me to make sure that subsequent versions 
> > correctly rely on the fixture teardown mechanism for child process termination.

> > So while your concern definitely makes sense, this specific case should be 
> > well-covered by the existing teardown logic.

> Are you sure the parent cleanup sees variables set in the child and
> actually takes effect?  We fork() the child so it should be a new VM
> which means that values stored there won't be seen by the parent, it'll
> see whatever values it had before the fork().

> It does also seem like bad practice to have duplicated cleanup code in
> both the test and the fixture cleanups, the fixture cleanup always runs
> so we should just use that all the time if it's in use (that's the whole
> idea really).  Indeed as things stand since the cleanup in the test
> doesn't reset self->child_pid so if the cleanup at the fixture level
> does anything there's a minor risk that we might race with the PID being
> reused and kill some new task 

> The fixture teardown handler also doesn't do the close(pidfd), either
> that's redundant for the in test cleanup or should be in the fixture
> (given that it's a child process it should be redundant as any open file
> descriptors are closed when the process exits).

You're absolutely right — thank you for pointing this out.

The reason for the redundant in-test cleanup was due to a previous issue during development (in v3), 
where an ASSERT_EQ() failure at the end of the test left the child process lingering, 
causing run_vmtests to hang. To address that at the time, I added explicit cleanup in the test body.

However, as you correctly observed, relying on test-local variables after fork() is flawed, 
and duplicating cleanup both in the test and in the fixture teardown is not ideal. 

The fixture teardown reliably handles cleanup and is the proper place for all resource release logic, 
including the close(pidfd) if it's needed.

I'll remove the redundant test-body cleanup and consolidate everything into the fixture teardown 
in the next version. 
Thanks again for the detailed and helpful review.

Best regards,
wang lian