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

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

Hi

sorry for late responce.

but I thought I should investigate BSD behavior at first.
and I've installed FreeBSD on my PC and tested it.

I'll post this test case and my proposal patch soon.


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

BSD inherit maxrss at exec(), but Jiri's patch doesn't.


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

xacct is linux specific feature.
it doesn't need other os compatibility.



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

your idea is nice. but it is incompatibility to bsd.
I beilieve incompatibility of compatibility feature is worthless.

BSD return 1GB at above case. then I like this behavior.




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

you are absolutely right.
I was crap thinking. thanks clarification.


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