Messages in this thread | | | Date | Sun, 4 Jan 2009 06:13:16 +0900 | From | "KOSAKI Motohiro" <> | Subject | Re: [PATCH for -mm] getrusage: fill ru_maxrss value |
| |
> sorry for delay! > >> >> Jiri's resend3 -> v1 >> - At wait_task_zombie(), parent process doesn't only collect child maxrss, >> but also cmaxrss. > > Ah yes, this looks very right to me.
hehe, therefore I like testcase driven development :)
>> - ru_maxrss inherit at exec() > > I must admit, I hate this ;)
Your feelings are understood well.
> That said, I agree with you point about compatibility. So I have to > agree with this change. > > Still, I'd like to know what other people think ;)
Yeah, me too.
> And I also agree that xacct is linux specific feature, but I still > I dislike the fact that xacct and getrusage report different numbers. > Perhaps we should change xacct as well?
I don't have any strong opinion. it seems good idea. but I don't know xacct so much.
> >> --- a/kernel/exit.c 2008-12-29 23:27:59.000000000 +0900 >> +++ b/kernel/exit.c 2008-12-31 21:08:08.000000000 +0900 >> @@ -1053,6 +1053,12 @@ NORET_TYPE void do_exit(long code) >> if (group_dead) { >> hrtimer_cancel(&tsk->signal->real_timer); >> exit_itimers(tsk->signal); >> + if (tsk->mm) { >> + unsigned long hiwater_rss = get_mm_hiwater_rss(tsk->mm); >> + >> + if (tsk->signal->maxrss < hiwater_rss) >> + tsk->signal->maxrss = hiwater_rss; >> + } > [...snip...] >> --- a/fs/exec.c 2008-12-25 08:26:37.000000000 +0900 >> +++ b/fs/exec.c 2008-12-31 21:11:28.000000000 +0900 >> @@ -870,6 +870,13 @@ static int de_thread(struct task_struct >> sig->notify_count = 0; >> >> no_thread_group: >> + if (current->mm) { >> + unsigned long hiwater_rss = get_mm_hiwater_rss(current->mm); >> + >> + if (sig->maxrss < hiwater_rss) >> + sig->maxrss = hiwater_rss; >> + } > > Perhaps it makes sense to factor out this code and make a helper? > > Unfortunately, exit_mm() and exec_mmap() do not have the common > path which can update sig->maxrss, mm_release() can't do this...
last problem are, function name and which file have it :)
>> + if (who != RUSAGE_CHILDREN) { >> + struct mm_struct *mm = get_task_mm(p); >> + if (mm) { >> + unsigned long hiwater_rss = get_mm_hiwater_rss(mm); >> + >> + if (maxrss < hiwater_rss) >> + maxrss = hiwater_rss; >> + mmput(mm); >> + } >> + } >> + r->ru_maxrss = maxrss * (PAGE_SIZE / 1024); /* convert pages to KBs */ > > Hmm... So, RUSAGE_THREAD always report maxrss == get_mm_hiwater_rss(mm) > and ignores signal->maxrss. Doesn't look right to me... > > Unless I missed something, Jiris's patch was fine, but given that now > we inherit maxrss at exec(), signal->maxrss can have the "inherited" > value?
you are right. thanks. will fix.
| |