lkml.org 
[lkml]   [2021]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 2/3] mm: introduce process_mrelease system call
    Date
    On 18.07.21 23:41, Suren Baghdasaryan wrote:
    > In modern systems it's not unusual to have a system component monitoring
    > memory conditions of the system and tasked with keeping system memory
    > pressure under control. One way to accomplish that is to kill
    > non-essential processes to free up memory for more important ones.
    > Examples of this are Facebook's OOM killer daemon called oomd and
    > Android's low memory killer daemon called lmkd.
    > For such system component it's important to be able to free memory
    > quickly and efficiently. Unfortunately the time process takes to free
    > up its memory after receiving a SIGKILL might vary based on the state
    > of the process (uninterruptible sleep), size and OPP level of the core
    > the process is running. A mechanism to free resources of the target
    > process in a more predictable way would improve system's ability to
    > control its memory pressure.
    > Introduce process_mrelease system call that releases memory of a dying
    > process from the context of the caller. This way the memory is freed in
    > a more controllable way with CPU affinity and priority of the caller.
    > The workload of freeing the memory will also be charged to the caller.
    > The operation is allowed only on a dying process.
    >
    > Previously I proposed a number of alternatives to accomplish this:
    > - https://lore.kernel.org/patchwork/patch/1060407 extending
    > pidfd_send_signal to allow memory reaping using oom_reaper thread;
    > - https://lore.kernel.org/patchwork/patch/1338196 extending
    > pidfd_send_signal to reap memory of the target process synchronously from
    > the context of the caller;
    > - https://lore.kernel.org/patchwork/patch/1344419/ to add MADV_DONTNEED
    > support for process_madvise implementing synchronous memory reaping.

    To me, this looks a lot cleaner. Although I do wonder why we need two
    separate mechanisms to achieve the end goal

    1. send sigkill
    2. process_mrelease

    As 2. doesn't make sense without 1. it somehow feels like it would be
    optimal to achieve both steps in a single syscall. But I remember there
    were discussions around that.

    >
    > The end of the last discussion culminated with suggestion to introduce a
    > dedicated system call (https://lore.kernel.org/patchwork/patch/1344418/#1553875)
    > The reasoning was that the new variant of process_madvise
    > a) does not work on an address range
    > b) is destructive
    > c) doesn't share much code at all with the rest of process_madvise
    > From the userspace point of view it was awkward and inconvenient to provide
    > memory range for this operation that operates on the entire address space.
    > Using special flags or address values to specify the entire address space
    > was too hacky.
    >
    > The API is as follows,
    >
    > int process_mrelease(int pidfd, unsigned int flags);
    >
    > DESCRIPTION
    > The process_mrelease() system call is used to free the memory of
    > a process which was sent a SIGKILL signal.
    >
    > The pidfd selects the process referred to by the PID file
    > descriptor.
    > (See pidofd_open(2) for further information)
    >
    > The flags argument is reserved for future use; currently, this
    > argument must be specified as 0.
    >
    > RETURN VALUE
    > On success, process_mrelease() returns 0. On error, -1 is
    > returned and errno is set to indicate the error.
    >
    > ERRORS
    > EBADF pidfd is not a valid PID file descriptor.
    >
    > EAGAIN Failed to release part of the address space.
    >
    > EINVAL flags is not 0.
    >
    > EINVAL The task does not have a pending SIGKILL or its memory is
    > shared with another process with no pending SIGKILL.
    >
    > ENOSYS This system call is not supported by kernels built with no
    > MMU support (CONFIG_MMU=n).
    >
    > ESRCH The target process does not exist (i.e., it has terminated
    > and been waited on).
    >
    > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
    > ---
    > mm/oom_kill.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
    > 1 file changed, 55 insertions(+)
    >
    > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
    > index d04a13dc9fde..7fbfa70d4e97 100644
    > --- a/mm/oom_kill.c
    > +++ b/mm/oom_kill.c
    > @@ -28,6 +28,7 @@
    > #include <linux/sched/task.h>
    > #include <linux/sched/debug.h>
    > #include <linux/swap.h>
    > +#include <linux/syscalls.h>
    > #include <linux/timex.h>
    > #include <linux/jiffies.h>
    > #include <linux/cpuset.h>
    > @@ -755,10 +756,64 @@ static int __init oom_init(void)
    > return 0;
    > }
    > subsys_initcall(oom_init)
    > +
    > +SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
    > +{
    > + struct pid *pid;
    > + struct task_struct *task;
    > + struct mm_struct *mm = NULL;
    > + unsigned int f_flags;
    > + long ret = 0;

    Nit: reverse Christmas tree.

    > +
    > + if (flags != 0)
    > + return -EINVAL;
    > +
    > + pid = pidfd_get_pid(pidfd, &f_flags);
    > + if (IS_ERR(pid))
    > + return PTR_ERR(pid);
    > +
    > + task = get_pid_task(pid, PIDTYPE_PID);
    > + if (!task) {
    > + ret = -ESRCH;
    > + goto put_pid;
    > + }
    > +
    > + /*
    > + * If the task is dying and in the process of releasing its memory
    > + * then get its mm.
    > + */
    > + task_lock(task);
    > + if (task_will_free_mem(task) && (task->flags & PF_KTHREAD) == 0) {
    > + mm = task->mm;
    > + mmget(mm);
    > + }

    AFAIU, while holding the task_lock, task->mm won't change and we cannot
    see a concurrent exit_mm()->mmput(). So the mm structure and the VMAs
    won't go away while holding the task_lock(). I do wonder if we need the
    mmget() at all here.

    Also, I wonder if it would be worth dropping the task_lock() while
    reaping - to unblock anybody else wanting to lock the task. Getting a
    hold of the mm and locking the mmap_lock would be sufficient I guess.


    In general, looks quite good to me.

    --
    Thanks,

    David / dhildenb

    \
     
     \ /
      Last update: 2021-07-21 10:07    [W:2.511 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site