Re: [LTP] [PATCH 2/3] Add process_mrelease01 test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp