Re: [PATCH v3 04/54] selftests/mm: migration: properly cleanup fork()ed processes

Luiz Capitulino
2026-04-30T13:38:05Z

On 2026-04-28 16:41, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> Several migration test use fork() to create worker processes. These
> processes are later killed, but nothing collects their exit status and they
> remain as zombies in the system.
>
> Add a helper function that kills the worker processes, waitpid()s for
> them and verifies the exit status.
>
> Replace the loops that call kill() for each process with a call to that
> helper.
>
> The addition of waitpid() calls also makes sure the migrating process never
> exits before the processes accessing the memory, so it's possible to drop
> the call to prctl(PR_SET_PDEATHSIG, SIGHUP).
>
> Reported-by: Luiz Capitulino <luizcap@redhat.com>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
> tools/testing/selftests/mm/migration.c | 54 +++++++++++++-------------
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
> index 3630f2fb0800..76e380b74658 100644
> --- a/tools/testing/selftests/mm/migration.c
> +++ b/tools/testing/selftests/mm/migration.c
> @@ -67,6 +67,24 @@ FIXTURE_TEARDOWN(migration)
> free(self->pids);
> }
>
> +static bool kill_children(FIXTURE_DATA(migration) * self)
> +{
> + int i, status;
> + pid_t pid;
> +
> + for (i = 0; i < self->nthreads; i++) {
> + pid = self->pids[i];
> + if (kill(pid, SIGTERM))
> + return false;
> + if (pid != waitpid(pid, &status, 0))
> + return false;
> + if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGTERM)
> + return false;
> + }

Why do we stop at the first error? Wouldn't it be better to kill &
collect all of them and then return false if at least one fails?

> +
> + return true;
> +}
> +
> int migrate(uint64_t *ptr, int n1, int n2)
> {
> int ret, tmp;
> @@ -160,20 +178,14 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
> memset(ptr, 0xde, TWOMEG);
> for (i = 0; i < self->nthreads; i++) {
> pid = fork();
> - if (!pid) {
> - prctl(PR_SET_PDEATHSIG, SIGHUP);
> - /* Parent may have died before prctl so check now. */
> - if (getppid() == 1)
> - kill(getpid(), SIGHUP);

This goes beyond this series, but I wonder if we should have this check
in access_mem() in case the parent dies in migrate().

> + if (!pid)
> access_mem(ptr);
> - } else {
> + else
> self->pids[i] = pid;
> - }
> }
>
> ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> - for (i = 0; i < self->nthreads; i++)
> - ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
> + ASSERT_EQ(kill_children(self), true);
> }
>
> /*
> @@ -236,20 +248,14 @@ TEST_F_TIMEOUT(migration, shared_anon_thp, 2*RUNTIME)
> memset(ptr, 0xde, pmdsize);
> for (i = 0; i < self->nthreads; i++) {
> pid = fork();
> - if (!pid) {
> - prctl(PR_SET_PDEATHSIG, SIGHUP);
> - /* Parent may have died before prctl so check now. */
> - if (getppid() == 1)
> - kill(getpid(), SIGHUP);
> + if (!pid)
> access_mem(ptr);
> - } else {
> + else
> self->pids[i] = pid;
> - }
> }
>
> ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> - for (i = 0; i < self->nthreads; i++)
> - ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
> + ASSERT_EQ(kill_children(self), true);
> }
>
> /*
> @@ -300,20 +306,14 @@ TEST_F_TIMEOUT(migration, shared_anon_htlb, 2*RUNTIME)
> memset(ptr, 0xde, hugepage_size);
> for (i = 0; i < self->nthreads; i++) {
> pid = fork();
> - if (!pid) {
> - prctl(PR_SET_PDEATHSIG, SIGHUP);
> - /* Parent may have died before prctl so check now. */
> - if (getppid() == 1)
> - kill(getpid(), SIGHUP);
> + if (!pid)
> access_mem(ptr);
> - } else {
> + else
> self->pids[i] = pid;
> - }
> }
>
> ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> - for (i = 0; i < self->nthreads; i++)
> - ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
> + ASSERT_EQ(kill_children(self), true);
> }
>
> TEST_HARNESS_MAIN