Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1427504 > unrolled thread

Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper

Started by"Hillf Danton" <hillf.zj@alibaba-inc.com>
First post2016-06-21 11:30 +0200
Last post2016-06-22 08:40 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-06-21 11:30 +0200
    Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into  a helper Michal Hocko <mhocko@kernel.org> - 2016-06-21 14:20 +0200
      Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-06-22 05:20 +0200
        Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into  a helper Michal Hocko <mhocko@kernel.org> - 2016-06-22 08:40 +0200

#1427504 — Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper

From"Hillf Danton" <hillf.zj@alibaba-inc.com>
Date2016-06-21 11:30 +0200
SubjectRe: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper
Message-ID<rMqal-3Th-9@gated-at.bofh.it>
> 
> From: Michal Hocko <mhocko@suse.com>
> 
> Currently we have two proc interfaces to set oom_score_adj. The legacy
> /proc/<pid>/oom_adj and /proc/<pid>/oom_score_adj which both have their
> specific handlers. Big part of the logic is duplicated so extract the
> common code into __set_oom_adj helper. Legacy knob still expects some
> details slightly different so make sure those are handled same way - e.g.
> the legacy mode ignores oom_score_adj_min and it warns about the usage.
> 
> This patch shouldn't introduce any functional changes.
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  fs/proc/base.c | 94 +++++++++++++++++++++++++++-------------------------------
>  1 file changed, 43 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 968d5ea06e62..a6a8fbdd5a1b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1037,7 +1037,47 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
>  	return simple_read_from_buffer(buf, count, ppos, buffer, len);
>  }
> 
> -static DEFINE_MUTEX(oom_adj_mutex);
> +static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> +{
> +	static DEFINE_MUTEX(oom_adj_mutex);

Writers are not excluded for readers!
Is this a hot path?

> +	struct task_struct *task;
> +	int err = 0;
> +
> +	task = get_proc_task(file_inode(file));
> +	if (!task)
> +		return -ESRCH;
> +
> +	mutex_lock(&oom_adj_mutex);
> +	if (legacy) {
> +		if (oom_adj < task->signal->oom_score_adj &&
> +				!capable(CAP_SYS_RESOURCE)) {
> +			err = -EACCES;
> +			goto err_unlock;
> +		}
> +		/*
> +		 * /proc/pid/oom_adj is provided for legacy purposes, ask users to use
> +		 * /proc/pid/oom_score_adj instead.
> +		 */
> +		pr_warn_once("%s (%d): /proc/%d/oom_adj is deprecated, please use /proc/%d/oom_score_adj instead.\n",
> +			  current->comm, task_pid_nr(current), task_pid_nr(task),
> +			  task_pid_nr(task));
> +	} else {
> +		if ((short)oom_adj < task->signal->oom_score_adj_min &&
> +				!capable(CAP_SYS_RESOURCE)) {
> +			err = -EACCES;
> +			goto err_unlock;
> +		}
> +	}
> +
> +	task->signal->oom_score_adj = oom_adj;
> +	if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> +		task->signal->oom_score_adj_min = (short)oom_adj;
> +	trace_oom_score_adj_update(task);
> +err_unlock:
> +	mutex_unlock(&oom_adj_mutex);
> +	put_task_struct(task);
> +	return err;
> +}
> 
>  /*
>   * /proc/pid/oom_adj exists solely for backwards compatibility with previous
> @@ -1052,7 +1092,6 @@ static DEFINE_MUTEX(oom_adj_mutex);
>  static ssize_t oom_adj_write(struct file *file, const char __user *buf,
>  			     size_t count, loff_t *ppos)
>  {
> -	struct task_struct *task;
>  	char buffer[PROC_NUMBUF];
>  	int oom_adj;
>  	int err;
> @@ -1074,12 +1113,6 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
>  		goto out;
>  	}
> 
> -	task = get_proc_task(file_inode(file));
> -	if (!task) {
> -		err = -ESRCH;
> -		goto out;
> -	}
> -
>  	/*
>  	 * Scale /proc/pid/oom_score_adj appropriately ensuring that a maximum
>  	 * value is always attainable.
> @@ -1089,26 +1122,7 @@ static ssize_t oom_adj_write(struct file *file, const char __user *buf,
>  	else
>  		oom_adj = (oom_adj * OOM_SCORE_ADJ_MAX) / -OOM_DISABLE;
> 
> -	mutex_lock(&oom_adj_mutex);
> -	if (oom_adj < task->signal->oom_score_adj &&
> -	    !capable(CAP_SYS_RESOURCE)) {
> -		err = -EACCES;
> -		goto err_unlock;
> -	}
> -
> -	/*
> -	 * /proc/pid/oom_adj is provided for legacy purposes, ask users to use
> -	 * /proc/pid/oom_score_adj instead.
> -	 */
> -	pr_warn_once("%s (%d): /proc/%d/oom_adj is deprecated, please use /proc/%d/oom_score_adj instead.\n",
> -		  current->comm, task_pid_nr(current), task_pid_nr(task),
> -		  task_pid_nr(task));
> -
> -	task->signal->oom_score_adj = oom_adj;
> -	trace_oom_score_adj_update(task);
> -err_unlock:
> -	mutex_unlock(&oom_adj_mutex);
> -	put_task_struct(task);
> +	err = __set_oom_adj(file, oom_adj, true);
>  out:
>  	return err < 0 ? err : count;
>  }
> @@ -1138,7 +1152,6 @@ static ssize_t oom_score_adj_read(struct file *file, char __user *buf,
>  static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
>  					size_t count, loff_t *ppos)
>  {
> -	struct task_struct *task;
>  	char buffer[PROC_NUMBUF];
>  	int oom_score_adj;
>  	int err;
> @@ -1160,28 +1173,7 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
>  		goto out;
>  	}
> 
> -	task = get_proc_task(file_inode(file));
> -	if (!task) {
> -		err = -ESRCH;
> -		goto out;
> -	}
> -
> -	mutex_lock(&oom_adj_mutex);
> -	if ((short)oom_score_adj < task->signal->oom_score_adj_min &&
> -			!capable(CAP_SYS_RESOURCE)) {
> -		err = -EACCES;
> -		goto err_unlock;
> -	}
> -
> -	task->signal->oom_score_adj = (short)oom_score_adj;
> -	if (has_capability_noaudit(current, CAP_SYS_RESOURCE))
> -		task->signal->oom_score_adj_min = (short)oom_score_adj;
> -
> -	trace_oom_score_adj_update(task);
> -
> -err_unlock:
> -	mutex_unlock(&oom_adj_mutex);
> -	put_task_struct(task);
> +	err = __set_oom_adj(file, oom_score_adj, false);
>  out:
>  	return err < 0 ? err : count;
>  }
> --
> 2.8.1
> 
>

[toc] | [next] | [standalone]

#1427693 — Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper

FromMichal Hocko <mhocko@kernel.org>
Date2016-06-21 14:20 +0200
SubjectRe: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper
Message-ID<rMsOR-5ID-11@gated-at.bofh.it>
In reply to#1427504
On Tue 21-06-16 17:27:57, Hillf Danton wrote:
> > 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > Currently we have two proc interfaces to set oom_score_adj. The legacy
> > /proc/<pid>/oom_adj and /proc/<pid>/oom_score_adj which both have their
> > specific handlers. Big part of the logic is duplicated so extract the
> > common code into __set_oom_adj helper. Legacy knob still expects some
> > details slightly different so make sure those are handled same way - e.g.
> > the legacy mode ignores oom_score_adj_min and it warns about the usage.
> > 
> > This patch shouldn't introduce any functional changes.
> > 
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  fs/proc/base.c | 94 +++++++++++++++++++++++++++-------------------------------
> >  1 file changed, 43 insertions(+), 51 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 968d5ea06e62..a6a8fbdd5a1b 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1037,7 +1037,47 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
> >  	return simple_read_from_buffer(buf, count, ppos, buffer, len);
> >  }
> > 
> > -static DEFINE_MUTEX(oom_adj_mutex);
> > +static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > +{
> > +	static DEFINE_MUTEX(oom_adj_mutex);
> 
> Writers are not excluded for readers!
> Is this a hot path?

I am not sure I follow you question. This is a write path... Who would
be the reader?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1428379

From"Hillf Danton" <hillf.zj@alibaba-inc.com>
Date2016-06-22 05:20 +0200
Message-ID<rMGRP-6mH-3@gated-at.bofh.it>
In reply to#1427693
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 968d5ea06e62..a6a8fbdd5a1b 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -1037,7 +1037,47 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
> > >  	return simple_read_from_buffer(buf, count, ppos, buffer, len);
> > >  }
> > >
> > > -static DEFINE_MUTEX(oom_adj_mutex);
> > > +static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > +{
> > > +	static DEFINE_MUTEX(oom_adj_mutex);
> >
> > Writers are not excluded for readers!
> > Is this a hot path?
> 
> I am not sure I follow you question. This is a write path... Who would
> be the reader?
> 
Currently oom_adj_read() and oom_adj_write() are serialized with 
task->sighand->siglock, and in this work oom_adj_mutex is introduced to
only keep writers in hose.

Plus, oom_adj_write() and oom_badness() are currently serialized 
with task->alloc_lock, and they may be handled in subsequent patches.

thanks
Hillf

[toc] | [prev] | [next] | [standalone]

#1428461 — Re: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper

FromMichal Hocko <mhocko@kernel.org>
Date2016-06-22 08:40 +0200
SubjectRe: [PATCH 03/10] proc, oom_adj: extract oom_score_adj setting into a helper
Message-ID<rMJZn-8iJ-7@gated-at.bofh.it>
In reply to#1428379
On Wed 22-06-16 11:17:12, Hillf Danton wrote:
> 
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 968d5ea06e62..a6a8fbdd5a1b 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1037,7 +1037,47 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
> > > >  	return simple_read_from_buffer(buf, count, ppos, buffer, len);
> > > >  }
> > > >
> > > > -static DEFINE_MUTEX(oom_adj_mutex);
> > > > +static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > > +{
> > > > +	static DEFINE_MUTEX(oom_adj_mutex);
> > >
> > > Writers are not excluded for readers!
> > > Is this a hot path?
> > 
> > I am not sure I follow you question. This is a write path... Who would
> > be the reader?
> > 
> Currently oom_adj_read() and oom_adj_write() are serialized with 
> task->sighand->siglock, and in this work oom_adj_mutex is introduced to
> only keep writers in hose.

OK, I see your point now. I didn't bother with the serialization with
readers because I believe it doesn't matter so much. Readers would
have to synchronize with writers to make sure they are seeing the most
current value otherwise you could see an outdated value anyway. It's
not like you would see a "corrupted" value without lock.

The primary point of the lock is to make sure that parallel updaters
cannot allow non-priviledged user to escape the restrictions.

If you see any specific scenario which would suffer from the lack of
serialization I can add the lock to readers as well.
 
> Plus, oom_adj_write() and oom_badness() are currently serialized 
> with task->alloc_lock, and they may be handled in subsequent patches.

alloc_lock is there just to make sure we see the proper mm.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web