Re: [patch v2] oom: fix oom_score_adj consistency with oom_disable_count

[Oleg Nesterov <oleg@redhat.com>]

On 11/03, David Rientjes wrote:
>
> On Wed, 3 Nov 2010, Oleg Nesterov wrote:
>
> > IOW. I believe that 3d5992d2ac7dc09aed8ab537cba074589f0f0a52
> > "oom: add per-mm oom disable count" should be reverted or fixed.
> >
> > Trivial example. A process with 2 threads, T1 and T2.
> > ->mm->oom_disable_count = 0.
> >
> > oom_score_adj_write() sets OOM_SCORE_ADJ_MIN and increments
> > oom_disable_count.
> >
> > T2 exits, notices OOM_SCORE_ADJ_MIN and decrements ->oom_disable_count
> > back to zero.
> >
> > Now, T1 runs with OOM_SCORE_ADJ_MIN, but its ->oom_disable_count == 0.
> >
> > No?
> >
>
> The intent of Ying's patch was for mm->oom_disable_count to map the number
> of threads sharing the ->mm that have p->signal->oom_score_adj ==
> OOM_SCORE_ADJ_MIN.

Yes, I see the intent. But the patch is obviouly wrong.

> > Another reason to move ->oom_score_adj into ->mm ;)
> >
>
> I would _love_ to move oom_score_adj into struct mm_struct, and I fought
> very strongly to do so,

Yes, I know ;)

> > Not sure this needs additional locking. exec_mmap() is called when
> > there are no other threads, we can rely on task_lock() we hold.
> >
>
> There are no other threads that can share tsk->signal at this point?  I
> was mislead by the de_thread() comment about CLONE_SIGHAND.

Agreed, the comment is misleading. "Other processes might share the signal
table" actually means: other processes (not only sub-threads) can share
->sighand. That is why de_thread() checks oldsighand->count at the end
of this function, after we already killed all sub-threads.

But at this point nobody except current uses this ->signal.

> > >  static int copy_mm(unsigned long clone_flags, struct task_struct * tsk)
> > >  {
> > >  	struct mm_struct * mm, *oldmm;
> > > +	unsigned long flags;
> > >  	int retval;
> > >
> > >  	tsk->min_flt = tsk->maj_flt = 0;
> > > @@ -743,8 +744,11 @@ good_mm:
> > >  	/* Initializing for Swap token stuff */
> > >  	mm->token_priority = 0;
> > >  	mm->last_interval = 0;
> > > -	if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > > -		atomic_inc(&mm->oom_disable_count);
> > > +	if (lock_task_sighand(tsk, &flags)) {
> > > +		if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > > +			atomic_inc(&mm->oom_disable_count);
> > > +		unlock_task_sighand(tsk, &flags);
> > > +	}
> >
> > This doesn't need ->siglock too. Nobody can see this new child,
> > nobody can access its tsk->signal.
>
> Ok!

OOPS! Sorry, I didn't notice that this code works in CLONE_VM|CLONE_THREAD
case too. In this case we do need the locking.

Wait. And what about the case I meant, !CLONE_THREAD case? In this case
we don't need ->siglock, but atomic_inc() is very wrong. Note that
this (new) mm_struct has the "random" value in ->oom_disable_count
copied from parent's ->mm.

> > > @@ -1700,13 +1707,19 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
> > >  		}
> > >
> > >  		if (new_mm) {
> > > +			unsigned long flags;
> > > +
> > >  			mm = current->mm;
> > >  			active_mm = current->active_mm;
> > >  			current->mm = new_mm;
> > >  			current->active_mm = new_mm;
> > > -			if (current->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> > > -				atomic_dec(&mm->oom_disable_count);
> > > -				atomic_inc(&new_mm->oom_disable_count);
> > > +			if (lock_task_sighand(current, &flags)) {
> > > +				if (current->signal->oom_score_adj ==
> > > +							OOM_SCORE_ADJ_MIN) {
> > > +					atomic_dec(&mm->oom_disable_count);
> > > +					atomic_inc(&new_mm->oom_disable_count);
> > > +				}
> >
> > This is racy anyway, even if we take ->siglock.
> >
> > If we need the protection from oom_score_adj_write(), then we have
> > to change ->mm under ->siglock as well. Otherwise, suppose that
> > oom_score_adj_write() sets OOM_SCORE_ADJ_MIN right after unshare()
> > does current->mm = new_mm.
> >
>
> We're protected by task_lock(current) in unshare, it can't do
> current->mm = new_mm while task_lock() is held in oom_score_adj_write().

Indeed, I was wrong, thanks. I forgot that this code actually never works
(if it worked, it should change ->mm for all sub-threads, each has its
 own task->alloc_lock).

> > However. Please do not touch this code. It doesn't work anyway,
> > I'll resend the patch which removes this crap.
> >
>
> Ok, I'll look forward to that :)

Sorry, don't have the time today. Will do tomorrow.

> Do you see issues with the mapping of threads attached to an mm being
> counted appropriately in mm->oom_disable_count?

Not sure I understand you.

The main problem is, they are not counted correctly. If exit_mm()
decrements this counter then oom_score_adj_write() should account
every live (with ->mm != NULL) thread, this is nasty. Or we should
find the way to drop the counter only when the whole process exits
(and in this case CLONE_THREAD shouldn't touch the counter).

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>