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

[Andrea Cervesato]

Ping @Cyril

On 10/10/24 14:33, Andrea Cervesato 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.
>>
>> 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