Re: [patch 1/3 v3] oom: add per-mm oom disable count

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

> > > @@ -745,6 +746,10 @@ static int exec_mmap(struct mm_struct *mm)
> > >  	tsk->mm = mm;
> > >  	tsk->active_mm = mm;
> > >  	activate_mm(active_mm, mm);
> > > +	if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> > > +		atomic_dec(&active_mm->oom_disable_count);
> > 
> > When kernel thread makes user-land process (e.g. usermode-helper),
> > active_mm might point to unrelated process. active_mm is only meaningful
> > for scheduler code. please don't touch it. probably you intend to
> > change old_mm.
> 
> This is safe because kthreads never have non-zero 
> p->signal->oom_score_adj.

Hm? my example is wrong? my point is, you shouldn't touch active_mm.


> > > @@ -1690,6 +1697,10 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
> > >  			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);
> > > +			}
> > >  			activate_mm(active_mm, new_mm);
> > >  			new_mm = mm;
> > >  		}
> > 
> > This place, we are grabbing task_lock(), but task_lock don't prevent
> > to change signal->oom_score_adj from another thread. This seems racy.
> > 
> 
> It does, task_lock(current) protects current->signal->oom_score_adj from 
> changing in oom-add-per-mm-oom-disable-count.patch.
> 
> I'll add the task_lock(p) in mm_init(), thanks for the review!

Wait, can you please elabolate more? task_lock() only lock one thread.
Why can it protect multi-thread race?



--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>