| Message ID | 20240522-process_mrelease-v1-2-41fe2fa44194@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add process_mrelease testing suite | expand |
Hi! > +/*\ > + * [Description] > + * > + * This test verifies that process_mrelease() syscall is releasing memory from > + * a killed process with memory allocation pending. > + */ > + > +#include "tst_test.h" > +#include "lapi/syscalls.h" > + > +#define CHUNK (1 * TST_MB) > +#define MAX_SIZE_MB (128 * TST_MB) > + > +static void *mem; > +static volatile int *mem_size; > + > +static void do_child(int size) > +{ > + tst_res(TINFO, "Child: allocate %d bytes", size); > + > + mem = SAFE_MMAP(NULL, > + size, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANON, > + 0, 0); This does not actually alloate any memory, it set ups the vmas but the actual memory is not allocated until you fault it by accessing it, so you have to actually touch every page to get it faulted, e.g. memset() it to something. > + TST_CHECKPOINT_WAKE_AND_WAIT(0); > + > + tst_res(TINFO, "Child: releasing memory"); > + > + SAFE_MUNMAP(mem, size); > +} > + > +static void run(void) > +{ > + int ret; > + int pidfd; > + int status; > + pid_t pid; > + volatile int restart; > + > + for (*mem_size = CHUNK; *mem_size < MAX_SIZE_MB; *mem_size += CHUNK) { > + restart = 0; > + > + pid = SAFE_FORK(); > + if (!pid) { > + do_child(*mem_size); > + exit(0); > + } > + > + TST_CHECKPOINT_WAIT(0); > + tst_disable_oom_protection(pid); What is this needed for? > + pidfd = SAFE_PIDFD_OPEN(pid, 0); > + > + tst_res(TINFO, "Parent: killing child with PID=%d", pid); > + > + SAFE_KILL(pid, SIGKILL); > + > + ret = tst_syscall(__NR_process_mrelease, pidfd, 0); > + if (ret == -1) { > + if (errno == ESRCH) { > + tst_res(TINFO, "Parent: child terminated before process_mrelease()." > + " Increase memory size and restart test"); > + > + restart = 1; As far as I understand the documentation his condition should not happen until you have called wait() on the process. > + } else { > + tst_res(TFAIL | TERRNO, "process_mrelease(%d,0) error", pidfd); > + } > + } else { > + tst_res(TPASS, "process_mrelease(%d,0) passed", pidfd); Okay it passed, but did we free any memory? Is the /proc/$pid/ still accessible before we wait on the killed process? If it is we can parse the proc maps before and after process_mrelease and check if the memory was really freed. > + } > + > + SAFE_WAITPID(-1, &status, 0); > + SAFE_CLOSE(pidfd); > + > + if (!restart) > + break; > + } > +} > + > +static void setup(void) > +{ > + mem_size = SAFE_MMAP( > + NULL, > + sizeof(int), > + PROT_READ | PROT_WRITE, > + MAP_SHARED | MAP_ANONYMOUS, > + -1, 0); Why do we keep the size in shared memory? The forked child has COW access to the parent memory, we can use the variables set in the parent just fine. > +} > + > +static void cleanup(void) > +{ > + if (mem) > + SAFE_MUNMAP(mem, *mem_size); This is allocated in chid and never non NULL in parent. > + if (mem_size) > + SAFE_MUNMAP((void *)mem_size, sizeof(int)); > +} > + > +static struct tst_test test = { > + .setup = setup, > + .cleanup = cleanup, > + .test_all = run, > + .needs_root = 1, > + .forks_child = 1, > + .min_kver = "5.15", > + .needs_checkpoints = 1, > + .needs_kconfigs = (const char *[]) { > + "CONFIG_MMU=y", > + }, > +}; > > -- > 2.35.3 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! On 7/17/24 15:14, Cyril Hrubis wrote: > Hi! >> +/*\ >> + * [Description] >> + * >> + * This test verifies that process_mrelease() syscall is releasing memory from >> + * a killed process with memory allocation pending. >> + */ >> + >> +#include "tst_test.h" >> +#include "lapi/syscalls.h" >> + >> +#define CHUNK (1 * TST_MB) >> +#define MAX_SIZE_MB (128 * TST_MB) >> + >> +static void *mem; >> +static volatile int *mem_size; >> + >> +static void do_child(int size) >> +{ >> + tst_res(TINFO, "Child: allocate %d bytes", size); >> + >> + mem = SAFE_MMAP(NULL, >> + size, >> + PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANON, >> + 0, 0); > This does not actually alloate any memory, it set ups the vmas but the > actual memory is not allocated until you fault it by accessing it, so > you have to actually touch every page to get it faulted, e.g. memset() > it to something. > Yes this is true. I will fix it. >> + TST_CHECKPOINT_WAKE_AND_WAIT(0); >> + >> + tst_res(TINFO, "Child: releasing memory"); >> + >> + SAFE_MUNMAP(mem, size); >> +} >> + >> +static void run(void) >> +{ >> + int ret; >> + int pidfd; >> + int status; >> + pid_t pid; >> + volatile int restart; >> + >> + for (*mem_size = CHUNK; *mem_size < MAX_SIZE_MB; *mem_size += CHUNK) { >> + restart = 0; >> + >> + pid = SAFE_FORK(); >> + if (!pid) { >> + do_child(*mem_size); >> + exit(0); >> + } >> + >> + TST_CHECKPOINT_WAIT(0); >> + tst_disable_oom_protection(pid); > What is this needed for? process_mrelease() overlaps with OOM, since it's used to free up thread memory instead of OOM. For this reason we need to disable it, so any OOM operation after kill() won't be taken by it. > >> + pidfd = SAFE_PIDFD_OPEN(pid, 0); >> + >> + tst_res(TINFO, "Parent: killing child with PID=%d", pid); >> + >> + SAFE_KILL(pid, SIGKILL); >> + >> + ret = tst_syscall(__NR_process_mrelease, pidfd, 0); >> + if (ret == -1) { >> + if (errno == ESRCH) { >> + tst_res(TINFO, "Parent: child terminated before process_mrelease()." >> + " Increase memory size and restart test"); >> + >> + restart = 1; > As far as I understand the documentation his condition should not happen > until you have called wait() on the process. The minimum requirement is that child has been killed and it's accessible from process_mrelease, so if we call wait() on the child, we won't be able to reach it anymore, causing ESRCH in process_mrelease(). Please check mrelease_test.c kselftest. > >> + } else { >> + tst_res(TFAIL | TERRNO, "process_mrelease(%d,0) error", pidfd); >> + } >> + } else { >> + tst_res(TPASS, "process_mrelease(%d,0) passed", pidfd); > Okay it passed, but did we free any memory? > > Is the /proc/$pid/ still accessible before we wait on the killed > process? If it is we can parse the proc maps before and after > process_mrelease and check if the memory was really freed. Good idea, I will do that. >> + } >> + >> + SAFE_WAITPID(-1, &status, 0); >> + SAFE_CLOSE(pidfd); >> + >> + if (!restart) >> + break; >> + } >> +} >> + >> +static void setup(void) >> +{ >> + mem_size = SAFE_MMAP( >> + NULL, >> + sizeof(int), >> + PROT_READ | PROT_WRITE, >> + MAP_SHARED | MAP_ANONYMOUS, >> + -1, 0); > Why do we keep the size in shared memory? The forked child has COW > access to the parent memory, we can use the variables set in the parent > just fine. +1 >> +} >> + >> +static void cleanup(void) >> +{ >> + if (mem) >> + SAFE_MUNMAP(mem, *mem_size); > This is allocated in chid and never non NULL in parent. > >> + if (mem_size) >> + SAFE_MUNMAP((void *)mem_size, sizeof(int)); >> +} >> + >> +static struct tst_test test = { >> + .setup = setup, >> + .cleanup = cleanup, >> + .test_all = run, >> + .needs_root = 1, >> + .forks_child = 1, >> + .min_kver = "5.15", >> + .needs_checkpoints = 1, >> + .needs_kconfigs = (const char *[]) { >> + "CONFIG_MMU=y", >> + }, >> +}; >> >> -- >> 2.35.3 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp Andrea
diff --git a/runtest/syscalls b/runtest/syscalls index cf06ee563..46a85fd31 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1050,6 +1050,8 @@ preadv203_64 preadv203_64 profil01 profil01 +process_mrelease01 process_mrelease01 + process_vm_readv01 process_vm01 -r process_vm_readv02 process_vm_readv02 process_vm_readv03 process_vm_readv03 diff --git a/testcases/kernel/syscalls/process_mrelease/.gitignore b/testcases/kernel/syscalls/process_mrelease/.gitignore new file mode 100644 index 000000000..673983858 --- /dev/null +++ b/testcases/kernel/syscalls/process_mrelease/.gitignore @@ -0,0 +1 @@ +/process_mrelease01 diff --git a/testcases/kernel/syscalls/process_mrelease/Makefile b/testcases/kernel/syscalls/process_mrelease/Makefile new file mode 100644 index 000000000..8cf1b9024 --- /dev/null +++ b/testcases/kernel/syscalls/process_mrelease/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/process_mrelease/process_mrelease01.c b/testcases/kernel/syscalls/process_mrelease/process_mrelease01.c new file mode 100644 index 000000000..aa7c9960c --- /dev/null +++ b/testcases/kernel/syscalls/process_mrelease/process_mrelease01.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + */ + +/*\ + * [Description] + * + * This test verifies that process_mrelease() syscall is releasing memory from + * a killed process with memory allocation pending. + */ + +#include "tst_test.h" +#include "lapi/syscalls.h" + +#define CHUNK (1 * TST_MB) +#define MAX_SIZE_MB (128 * TST_MB) + +static void *mem; +static volatile int *mem_size; + +static void do_child(int size) +{ + tst_res(TINFO, "Child: allocate %d bytes", size); + + mem = SAFE_MMAP(NULL, + size, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANON, + 0, 0); + + TST_CHECKPOINT_WAKE_AND_WAIT(0); + + tst_res(TINFO, "Child: releasing memory"); + + SAFE_MUNMAP(mem, size); +} + +static void run(void) +{ + int ret; + int pidfd; + int status; + pid_t pid; + volatile int restart; + + for (*mem_size = CHUNK; *mem_size < MAX_SIZE_MB; *mem_size += CHUNK) { + restart = 0; + + pid = SAFE_FORK(); + if (!pid) { + do_child(*mem_size); + exit(0); + } + + TST_CHECKPOINT_WAIT(0); + + tst_disable_oom_protection(pid); + + pidfd = SAFE_PIDFD_OPEN(pid, 0); + + tst_res(TINFO, "Parent: killing child with PID=%d", pid); + + SAFE_KILL(pid, SIGKILL); + + ret = tst_syscall(__NR_process_mrelease, pidfd, 0); + if (ret == -1) { + if (errno == ESRCH) { + tst_res(TINFO, "Parent: child terminated before process_mrelease()." + " Increase memory size and restart test"); + + restart = 1; + } else { + tst_res(TFAIL | TERRNO, "process_mrelease(%d,0) error", pidfd); + } + } else { + tst_res(TPASS, "process_mrelease(%d,0) passed", pidfd); + } + + SAFE_WAITPID(-1, &status, 0); + SAFE_CLOSE(pidfd); + + if (!restart) + break; + } +} + +static void setup(void) +{ + mem_size = SAFE_MMAP( + NULL, + sizeof(int), + PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, + -1, 0); +} + +static void cleanup(void) +{ + if (mem) + SAFE_MUNMAP(mem, *mem_size); + + if (mem_size) + SAFE_MUNMAP((void *)mem_size, sizeof(int)); +} + +static struct tst_test test = { + .setup = setup, + .cleanup = cleanup, + .test_all = run, + .needs_root = 1, + .forks_child = 1, + .min_kver = "5.15", + .needs_checkpoints = 1, + .needs_kconfigs = (const char *[]) { + "CONFIG_MMU=y", + }, +};