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

[Wei Gao via ltp <ltp@lists.linux.it>]

On Thu, Oct 10, 2024 at 02:33:09PM +0200, Andrea Cervesato via ltp wrote:
> Hi!
> 
> On 9/12/24 18:28, Cyril Hrubis wrote:
> > 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.
If you add extra sleep between SAFE_KILL(pid, SIGKILL) and tst_syscall
you will get ESRCH, so i guess child process has chance to be fully
reaped before process_mrelease syscall.
> > 
> > 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...
> I don't know, I tried to port the kselftest that seemed to be reasonable.
> Let me know if this is still good, otherwise we need to change the whole
> algorithm. But honestly I don't see many other options than the current one.
kselftest does not has this memory check, i have done some test in my
env, this memory check can pass even without the process_mrelease, so i
think we do not need this check.
> > 
> > > +		}
> > > +
> > > +		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
> Andrea
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

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