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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static void run(void)
> +{
> +	int ret;
> +	int pidfd;
> +	int status;
> +	pid_t pid;
> +	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);
> +
> +		if (!memory_is_mapped(pid, *mem_addr, *mem_addr + mem_size)) {
> +			tst_res(TFAIL, "Memory is not mapped");
> +			break;
> +		}
> +
> +		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;

Does this even happen? I suppose that until the child has been waited
for you shouldn't get ESRCH at all. The memory may be freed
asynchronously but the pidfd is valid until we do waitpid, at least
that's what the description says:

https://lore.kernel.org/linux-mm/20210902220029.bfau3YxNP%25akpm@linux-foundation.org/

But selftest seems to do the same loop on ESRCH so either the test or
the documentation is wrong.

Michal any idea which is correct?

> +			} else {
> +				tst_res(TFAIL | TERRNO, "process_mrelease(%d,0) error", pidfd);
> +			}
> +		} else {
> +			int timeout_ms = 1000;
> +
> +			tst_res(TPASS, "process_mrelease(%d,0) passed", pidfd);
> +
> +			while (memory_is_mapped(pid, *mem_addr, *mem_addr + mem_size) &&
> +				timeout_ms--)
> +				usleep(1000);
> +
> +			if (memory_is_mapped(pid, *mem_addr, *mem_addr + mem_size))
> +				tst_res(TFAIL, "Memory is still mapped inside child memory");
> +			else
> +				tst_res(TPASS, "Memory has been released");

As far as I understand this this will likely pass even without the
process_mrelease() call since the process address space is being teared
down anyways. But I do not have an idea how to make things better. I
guess that if we wanted to know for sure we would have to run some
complex statistics with and without the syscall and compare the
timings...


> +		}
> +
> +		SAFE_WAITPID(-1, &status, 0);
> +		SAFE_CLOSE(pidfd);
> +
> +		if (!restart)
> +			break;
> +	}
> +}
> +
> +static void setup(void)
> +{
> +	mem_addr = SAFE_MMAP(NULL,
> +		sizeof(unsigned long),
> +		PROT_READ | PROT_WRITE,
> +		MAP_SHARED | MAP_ANON,
> +		0, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (mem_addr)
> +		SAFE_MUNMAP(mem_addr, sizeof(unsigned long));
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.min_kver = "5.15",
> +	.needs_checkpoints = 1,
> +};
> 
> -- 
> 2.43.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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