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

From: Oleg Nesterov
Date: Thu Dec 25 2008 - 09:44:59 EST


On 12/25, KOSAKI Motohiro wrote:
>
> > On Wed, 24 Dec 2008 16:37:55 +0900 (JST)
> > KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/