lkml.org 
[lkml]   [2009]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [patch -mm v2] mm: introduce oom_adj_child
    Date
    NAK this. I explain the reason at below.


    > It's helpful to be able to specify an oom_adj value for newly forked
    > children that do not share memory with the parent.
    >
    > Before making oom_adj values a characteristic of a task's mm in
    > 2ff05b2b4eac2e63d345fc731ea151a060247f53, it was possible to change the
    > oom_adj value of a vfork() child prior to execve() without implicitly
    > changing the oom_adj value of the parent. With the new behavior, the
    > oom_adj values of both threads would change since they represent the same
    > memory.
    >
    > That change was necessary to fix an oom killer livelock which would occur
    > when a child would be selected for oom kill prior to execve() and the
    > task could not be killed because it shared memory with an OOM_DISABLE
    > parent. In fact, only the most negative (most immune) oom_adj value for
    > all threads sharing the same memory would actually be used by the oom
    > killer, leaving inconsistencies amongst all other threads having
    > different oom_adj values (and, thus, incorrectly exported
    > /proc/pid/oom_score values).
    >
    > This patch adds a new per-process parameter: /proc/pid/oom_adj_child.

    nit: per-thread.

    > This defaults to mirror the value of /proc/pid/oom_adj but may be changed
    > so that mm's initialized by their children are preferred over the parent
    > by the oom killer. Setting oom_adj_child to be less (i.e. more immune)
    > than the task's oom_adj value itself is governed by the CAP_SYS_RESOURCE
    > capability.
    >
    > When a mm is initialized, the initial oom_adj value will be set to the
    > parent's oom_adj_child. This allows tasks to elevate the oom_adj value
    > of a vfork'd child prior to execve() before the execution actually takes
    > place.
    >
    > Furthermore, /proc/pid/oom_adj_child is inherited from the task that
    > forked it.
    >
    > Cc: Rik van Riel <riel@redhat.com>
    > Cc: Paul Menage <menage@google.com>
    > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    > Signed-off-by: David Rientjes <rientjes@google.com>
    > ---
    > Documentation/filesystems/proc.txt | 38 ++++++++++++++++----
    > fs/proc/base.c | 68 ++++++++++++++++++++++++++++++++++++
    > include/linux/sched.h | 1 +
    > kernel/fork.c | 3 +-
    > 4 files changed, 101 insertions(+), 9 deletions(-)
    >
    > diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
    > --- a/Documentation/filesystems/proc.txt
    > +++ b/Documentation/filesystems/proc.txt
    > @@ -34,10 +34,11 @@ Table of Contents
    >
    > 3 Per-Process Parameters
    > 3.1 /proc/<pid>/oom_adj - Adjust the oom-killer score
    > - 3.2 /proc/<pid>/oom_score - Display current oom-killer score
    > - 3.3 /proc/<pid>/io - Display the IO accounting fields
    > - 3.4 /proc/<pid>/coredump_filter - Core dump filtering settings
    > - 3.5 /proc/<pid>/mountinfo - Information about mounts
    > + 3.2 /proc/<pid>/oom_adj_child - Change default oom_adj for children
    > + 3.3 /proc/<pid>/oom_score - Display current oom-killer score
    > + 3.4 /proc/<pid>/io - Display the IO accounting fields
    > + 3.5 /proc/<pid>/coredump_filter - Core dump filtering settings
    > + 3.6 /proc/<pid>/mountinfo - Information about mounts
    >
    >
    > ------------------------------------------------------------------------------
    > @@ -1206,7 +1207,28 @@ The task with the highest badness score is then selected and its children
    > are killed, process itself will be killed in an OOM situation when it does
    > not have children or some of them disabled oom like described above.
    >
    > -3.2 /proc/<pid>/oom_score - Display current oom-killer score
    > +
    > +3.2 /proc/<pid>/oom_adj_child - Change default oom_adj for children
    > +-------------------------------------------------------------------
    > +
    > +This file can be used to change the default oom_adj value for children when a
    > +new mm is initialized. The oom_adj value for a child's mm is typically the
    > +task's oom_adj value itself, however this value can be altered by writing to
    > +this file.
    > +
    > +This is particularly helpful when a child is vfork'd and its mm following exec
    > +should have a higher priority oom_adj value than its parent. The new mm will
    > +default to oom_adj_child of the parent task.
    > +
    > +oom_adj_child will mirror oom_adj whenever the latter changes for all tasks
    > +that share its memory. This avoids having to set both values when simply
    > +tuning oom_adj and that value should be inherited by all children.
    > +
    > +Setting oom_adj_child to be more immune than the task's mm itself (i.e. less
    > +than oom_adj) is governed by the CAP_SYS_RESOURCE capability.
    > +
    > +
    > +3.3 /proc/<pid>/oom_score - Display current oom-killer score
    > -------------------------------------------------------------
    >
    > This file can be used to check the current score used by the oom-killer is for
    > @@ -1214,7 +1236,7 @@ any given <pid>. Use it together with /proc/<pid>/oom_adj to tune which
    > process should be killed in an out-of-memory situation.
    >
    >
    > -3.3 /proc/<pid>/io - Display the IO accounting fields
    > +3.4 /proc/<pid>/io - Display the IO accounting fields
    > -------------------------------------------------------
    >
    > This file contains IO statistics for each running process
    > @@ -1316,7 +1338,7 @@ those 64-bit counters, process A could see an intermediate result.
    > More information about this can be found within the taskstats documentation in
    > Documentation/accounting.
    >
    > -3.4 /proc/<pid>/coredump_filter - Core dump filtering settings
    > +3.5 /proc/<pid>/coredump_filter - Core dump filtering settings
    > ---------------------------------------------------------------
    > When a process is dumped, all anonymous memory is written to a core file as
    > long as the size of the core file isn't limited. But sometimes we don't want
    > @@ -1360,7 +1382,7 @@ For example:
    > $ echo 0x7 > /proc/self/coredump_filter
    > $ ./some_program
    >
    > -3.5 /proc/<pid>/mountinfo - Information about mounts
    > +3.6 /proc/<pid>/mountinfo - Information about mounts
    > --------------------------------------------------------
    >
    > This file contains lines of the form:
    > diff --git a/fs/proc/base.c b/fs/proc/base.c
    > --- a/fs/proc/base.c
    > +++ b/fs/proc/base.c
    > @@ -1023,6 +1023,7 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
    > size_t count, loff_t *ppos)
    > {
    > struct task_struct *task;
    > + struct task_struct *g, *p;
    > char buffer[PROC_NUMBUF], *end;
    > int oom_adjust;
    >
    > @@ -1051,6 +1052,12 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
    > put_task_struct(task);
    > return -EACCES;
    > }
    > + read_lock(&tasklist_lock);
    > + do_each_thread(g, p) {
    > + if (p->mm && p->mm == task->mm)
    > + p->oom_adj_child = oom_adjust;
    > + } while_each_thread(g, p);
    > + read_unlock(&tasklist_lock);
    > task->mm->oom_adj = oom_adjust;
    > task_unlock(task);
    > put_task_struct(task);
    > @@ -1064,6 +1071,65 @@ static const struct file_operations proc_oom_adjust_operations = {
    > .write = oom_adjust_write,
    > };
    >
    > +static ssize_t oom_adj_child_read(struct file *file, char __user *buf,
    > + size_t count, loff_t *ppos)
    > +{
    > + struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
    > + char buffer[PROC_NUMBUF];
    > + size_t len;
    > + int oom_adj_child;
    > +
    > + if (!task)
    > + return -ESRCH;
    > + oom_adj_child = task->oom_adj_child;
    > + put_task_struct(task);
    > +
    > + len = snprintf(buffer, sizeof(buffer), "%i\n", oom_adj_child);
    > +
    > + return simple_read_from_buffer(buf, count, ppos, buffer, len);
    > +}
    > +
    > +static ssize_t oom_adj_child_write(struct file *file, const char __user *buf,
    > + size_t count, loff_t *ppos)
    > +{
    > + struct task_struct *task;
    > + char buffer[PROC_NUMBUF], *end;
    > + int oom_adj_child;
    > +
    > + memset(buffer, 0, sizeof(buffer));
    > + if (count > sizeof(buffer) - 1)
    > + count = sizeof(buffer) - 1;
    > + if (copy_from_user(buffer, buf, count))
    > + return -EFAULT;
    > + oom_adj_child = simple_strtol(buffer, &end, 0);
    > + if ((oom_adj_child < OOM_ADJUST_MIN ||
    > + oom_adj_child > OOM_ADJUST_MAX) && oom_adj_child != OOM_DISABLE)
    > + return -EINVAL;
    > + if (*end == '\n')
    > + end++;
    > + task = get_proc_task(file->f_path.dentry->d_inode);
    > + if (!task)
    > + return -ESRCH;
    > + task_lock(task);
    > + if (task->mm && oom_adj_child < task->mm->oom_adj &&
    > + !capable(CAP_SYS_RESOURCE)) {
    > + task_unlock(task);
    > + put_task_struct(task);
    > + return -EINVAL;
    > + }
    > + task_unlock(task);
    > + task->oom_adj_child = oom_adj_child;
    > + put_task_struct(task);
    > + if (end - buffer == 0)
    > + return -EIO;
    > + return end - buffer;
    > +}
    > +
    > +static const struct file_operations proc_oom_adj_child_operations = {
    > + .read = oom_adj_child_read,
    > + .write = oom_adj_child_write,
    > +};
    > +
    > #ifdef CONFIG_AUDITSYSCALL
    > #define TMPBUFLEN 21
    > static ssize_t proc_loginuid_read(struct file * file, char __user * buf,
    > @@ -2548,6 +2614,7 @@ static const struct pid_entry tgid_base_stuff[] = {
    > #endif
    > INF("oom_score", S_IRUGO, proc_oom_score),
    > REG("oom_adj", S_IRUGO|S_IWUSR, proc_oom_adjust_operations),
    > + REG("oom_adj_child", S_IRUGO|S_IWUSR, proc_oom_adj_child_operations),
    > #ifdef CONFIG_AUDITSYSCALL
    > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
    > REG("sessionid", S_IRUGO, proc_sessionid_operations),
    > @@ -2886,6 +2953,7 @@ static const struct pid_entry tid_base_stuff[] = {
    > #endif
    > INF("oom_score", S_IRUGO, proc_oom_score),
    > REG("oom_adj", S_IRUGO|S_IWUSR, proc_oom_adjust_operations),
    > + REG("oom_adj_child", S_IRUGO|S_IWUSR, proc_oom_adj_child_operations),
    > #ifdef CONFIG_AUDITSYSCALL
    > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
    > REG("sessionid", S_IRUSR, proc_sessionid_operations),
    > diff --git a/include/linux/sched.h b/include/linux/sched.h
    > --- a/include/linux/sched.h
    > +++ b/include/linux/sched.h
    > @@ -1198,6 +1198,7 @@ struct task_struct {
    > * a short time
    > */
    > unsigned char fpu_counter;
    > + s8 oom_adj_child; /* Default child OOM-kill score adjustment */
    > #ifdef CONFIG_BLK_DEV_IO_TRACE
    > unsigned int btrace_seq;
    > #endif
    > diff --git a/kernel/fork.c b/kernel/fork.c
    > --- a/kernel/fork.c
    > +++ b/kernel/fork.c
    > @@ -426,7 +426,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
    > init_rwsem(&mm->mmap_sem);
    > INIT_LIST_HEAD(&mm->mmlist);
    > mm->flags = (current->mm) ? current->mm->flags : default_dump_filter;
    > - mm->oom_adj = (current->mm) ? current->mm->oom_adj : 0;
    > + mm->oom_adj = p->oom_adj_child;

    This code doesn't fix anything.
    mm->oom_adj assignment still change vfork() parent process oom_adj value.
    (Again, vfork() parent and child use the same mm)

    IOW, in vfork case, oom_adj_child parameter doesn't only change child oom_adj,
    but also parent oom_adj value.
    IOW, oom_adj_child is NOT child effective parameter.


    > mm->core_state = NULL;
    > mm->nr_ptes = 0;
    > set_mm_counter(mm, file_rss, 0);
    > @@ -679,6 +679,7 @@ good_mm:
    >
    > tsk->mm = mm;
    > tsk->active_mm = mm;
    > + tsk->oom_adj_child = mm->oom_adj;
    > return 0;
    >
    > fail_nomem:





    \
     
     \ /
      Last update: 2009-07-30 04:35    [W:0.043 / U:1.400 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site