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

Andrea Cervesato via ltp
2024-10-10T12:33:40Z

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.
>
> 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.
>
>> + }
>> +
>> + 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