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

[Michal Hocko]

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