Re: [PATCH 4/5] mm, oom_reaper: report success/failure

[Michal Hocko]

On Wed 03-02-16 15:10:57, David Rientjes wrote:
> On Wed, 3 Feb 2016, Michal Hocko wrote:
> 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 8e345126d73e..b87acdca2a41 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -420,6 +420,7 @@ static struct task_struct *oom_reaper_th;
> >  static struct task_struct *task_to_reap;
> >  static DECLARE_WAIT_QUEUE_HEAD(oom_reaper_wait);
> >  
> > +#define K(x) ((x) << (PAGE_SHIFT-10))
> >  static bool __oom_reap_task(struct task_struct *tsk)
> >  {
> >  	struct mmu_gather tlb;
> > @@ -476,6 +477,11 @@ static bool __oom_reap_task(struct task_struct *tsk)
> >  		}
> >  	}
> >  	tlb_finish_mmu(&tlb, 0, -1);
> > +	pr_info("oom_reaper: reaped process :%d (%s) anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lulB\n",
> > +			task_pid_nr(tsk), tsk->comm,
> > +			K(get_mm_counter(mm, MM_ANONPAGES)),
> > +			K(get_mm_counter(mm, MM_FILEPAGES)),
> > +			K(get_mm_counter(mm, MM_SHMEMPAGES)));
> >  	up_read(&mm->mmap_sem);
> >  
> >  	/*
> 
> This is a bit misleading, it would appear that the rss values are what was 
> reaped when in fact they represent just the values of the mm being reaped.  
> We have already printed these values as an artifact in the kernel log.

Yes and the idea was to provide the after state to compare before and
after. That's why I have kept the similar format. Just dropped the
virtual memory size because that doesn't make any sense in this context
now.

> I think it would be helpful to show anon-rss after reaping, however, so we 
> can compare to the previous anon-rss that was reported.  And, I agree that 
> leaving behind a message in the kernel log that reaping has been 
> successful is worthwhile.  So this line should just show what anon-rss is 
> after reaping and make it clear that this is not the memory reaped.

Does
"oom_reaper: reaped process %d (%s) current memory anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB "

sound any better?
-- 
Michal Hocko
SUSE Labs