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

[David Rientjes <rientjes@google.com>]

On Mon, 30 Aug 2010, KOSAKI Motohiro wrote:

> > diff --git a/fs/exec.c b/fs/exec.c
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -54,6 +54,7 @@
> >  #include <linux/fsnotify.h>
> >  #include <linux/fs_struct.h>
> >  #include <linux/pipe_fs_i.h>
> > +#include <linux/oom.h>
> >  
> >  #include <asm/uaccess.h>
> >  #include <asm/mmu_context.h>
> > @@ -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.

> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -310,6 +310,8 @@ struct mm_struct {
> >  #ifdef CONFIG_MMU_NOTIFIER
> >  	struct mmu_notifier_mm *mmu_notifier_mm;
> >  #endif
> > +	/* How many tasks sharing this mm are OOM_DISABLE */
> > +	atomic_t oom_disable_count;
> >  };
> >  
> >  /* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -50,6 +50,7 @@
> >  #include <linux/perf_event.h>
> >  #include <trace/events/sched.h>
> >  #include <linux/hw_breakpoint.h>
> > +#include <linux/oom.h>
> >  
> >  #include <asm/uaccess.h>
> >  #include <asm/unistd.h>
> > @@ -689,6 +690,8 @@ static void exit_mm(struct task_struct * tsk)
> >  	enter_lazy_tlb(mm, current);
> >  	/* We don't want this task to be frozen prematurely */
> >  	clear_freeze_flag(tsk);
> > +	if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > +		atomic_dec(&mm->oom_disable_count);
> >  	task_unlock(tsk);
> >  	mm_update_next_owner(mm);
> >  	mmput(mm);
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -65,6 +65,7 @@
> >  #include <linux/perf_event.h>
> >  #include <linux/posix-timers.h>
> >  #include <linux/user-return-notifier.h>
> > +#include <linux/oom.h>
> >  
> >  #include <asm/pgtable.h>
> >  #include <asm/pgalloc.h>
> > @@ -485,6 +486,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
> >  	mm->cached_hole_size = ~0UL;
> >  	mm_init_aio(mm);
> >  	mm_init_owner(mm, p);
> > +	atomic_set(&mm->oom_disable_count, 0);
> >  
> >  	if (likely(!mm_alloc_pgd(mm))) {
> >  		mm->def_flags = 0;
> > @@ -738,6 +740,8 @@ 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);
> >  
> >  	tsk->mm = mm;
> >  	tsk->active_mm = mm;
> > @@ -1296,8 +1300,11 @@ bad_fork_cleanup_io:
> >  bad_fork_cleanup_namespaces:
> >  	exit_task_namespaces(p);
> >  bad_fork_cleanup_mm:
> > -	if (p->mm)
> > +	if (p->mm) {
> > +		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > +			atomic_dec(&p->mm->oom_disable_count);
> >  		mmput(p->mm);
> > +	}
> 
> This place, we don't have any lock. so, checking signal->oom_score_adj and
> change oom_disable_count seems inatomic.
> 

Ah, true, we need task_lock(p) around the conditional, thanks.

> >  bad_fork_cleanup_signal:
> >  	if (!(clone_flags & CLONE_THREAD))
> >  		free_signal_struct(p->signal);
> > @@ -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!

--
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>