On Wed, 1 Sep 2010, KOSAKI Motohiro wrote:
> > > > @@ -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.
>
Ok, I'll use old_mm instead as a cleanup. Thanks!
> > > > @@ -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?
>
We take task_lock(tsk) whenever we change tsk->signal->oom_score_adj.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>