Re: [PATCH, RESEND3] getrusage: fill ru_maxrss value

[Oleg Nesterov <oleg@redhat.com>]

On 12/25, KOSAKI Motohiro wrote:
>
> > On Wed, 24 Dec 2008 16:37:55 +0900 (JST)
> > KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> > 
> > > > @@ -870,6 +870,7 @@ static int de_thread(struct task_struct *tsk)
> > > >  	sig->notify_count = 0;
> > > >  
> > > >  no_thread_group:
> > > > +	sig->maxrss = 0;
> > > >  	exit_itimers(sig);
> > > >  	flush_itimer_signals();
> > > >  	if (leader)
> > > 
> > > I don't know getrusage correct behavior so detail.
> > > Why don't update parent process's sig->cmaxrss ?
> > Because exec affects only this task and we want to forgot maxrss value.
> > That does not implicate that we want to forgot highest maxrss value of
> > our childs because exec does not affect them. I think this is right
> > behavior.
> 
> Hmm, "we want" is a bit ambiguously word.
> We have three reviewing viewpoint.
> 
> 1) this code is consistent to other linux kernel code.
> 
> this patch fill its requrement perfectly.
> 
> 2) the behavior is enough surpriseless?
> 
> Honestly, I think this patch is a bit supriseful.
> example,
> 
>   1. process-A fork process-B
>   2. process-A wait by wait4()
>   3. process-B consume 1GB memory
>   4. process-B exec another program
>   5. process-B consume 100KB memory
>   6. process-B exit
>   7. process-A get maxrss=100KB
> 
> oh, 1GB consumption is disappeared.

Yes, and I also think this is not right.

But this has nothing to do with parent->signal->cmaxrss, the question
is why we lost the info.

And please note we have the same problem with xacct_add_tsk(), that
is why I think this patch is right _for now_, but see below.

> However, if we choice process don't forget maxrss at exec,
> another supriseful happend.
> example,
> 
>   1. process-A consume 1GB memroy
>   2. process-A fork process-B
>   3. process-A wait by wait4()
>   4. right after, process-B exec another program
>   5. process-B consume 100KB memory
>   6. process-B exit
>   7. process-A get maxrss=1GB
> 
> oh, this design cause large process can't get child maxrss.

Even simpler. If we never reset marxrss, we start inherit this
value from /sbin/init which is obviously wrong.

That is why I suggested to change bprm_mm_init's path to keep
mm->hiwater_xxx but only if !PF_FORKNOEXEC (and in that case
we must kill "sig->maxrss = 0" from de_thread()).

What do you think?

> if who==RUSAGE_BOTH,

Ah. I forgot to mention what the code does to discuss...

> correct behavior: max(hiwater_rss, mm_rss) + signal->cmaxrss)

I disagree. This doesn't look correct to me. ->maxrss is a maximum,
we shouldn't report the sum.

> this code:        max(signal->maxrss + signal->cmaxrss, max(hiwater_rss, mm_rss))

Afaics, no. this code reports

	max(signal->cmaxrss, max(signal->maxrss, get_mm_hiwater_rss())

and this looks right to me.

But I agree, this is debateable.

Oleg.