* [PATCH, RESEND2] getrusage: fill ru_maxrss value @ 2008-12-17 9:56 Jiri Pirko [not found] ` <20081217105639.63e765eb-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2008-12-17 9:56 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api-u79uwXL29TY76Z2rM5mHXA, Hugh Dickins, jlim-sJ/iWh9BUns, kosaki.motohiro-+CUm20s59erQFUHtdCDX3A (updated) This patch makes ->ru_maxrss value in struct rusage filled accordingly to rss hiwater mark. This struct is filled as a parameter to getrusage syscall. ->ru_maxrss value is set to pages which might be correct as "time" application converts it to KBs. To make this happen we extend struct signal_struct by two fields. The first one is ->maxrss which we use to store rss hiwater of the task. The second one is ->cmaxrss which we use to store highest rss hiwater of all task childs. These values are used in k_getrusage() to actually fill ->ru_maxrss. k_getrusage() uses current rss hiwater value directly if mm struct exists. We clear the ->maxrss as a part of flush_old_exec() to be consistent because bprm_mm_init() does not copy ->hiwater_rss. Note that we use recently introduced get_mm_hiwater_rss() helper to actually get the rss hiwater value: http://lkml.org/lkml/2008/12/12/172 Signed-off-by: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> --- fs/exec.c | 1 + include/linux/sched.h | 1 + kernel/exit.c | 4 ++++ kernel/fork.c | 1 + kernel/sys.c | 14 ++++++++++++++ 5 files changed, 21 insertions(+), 0 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index ec5df9a..8d3d0f9 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -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) diff --git a/include/linux/sched.h b/include/linux/sched.h index f4c70dc..41b04ee 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -563,6 +563,7 @@ struct signal_struct { unsigned long min_flt, maj_flt, cmin_flt, cmaj_flt; unsigned long inblock, oublock, cinblock, coublock; struct task_io_accounting ioac; + unsigned long maxrss, cmaxrss; /* * We don't bother to synchronize most readers of this at all, diff --git a/kernel/exit.c b/kernel/exit.c index 81b6372..61d622d 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1053,6 +1053,8 @@ NORET_TYPE void do_exit(long code) if (group_dead) { hrtimer_cancel(&tsk->signal->real_timer); exit_itimers(tsk->signal); + if (tsk->mm) + tsk->signal->maxrss = get_mm_hiwater_rss(tsk->mm); } acct_collect(code, group_dead); if (group_dead) @@ -1351,6 +1353,8 @@ static int wait_task_zombie(struct task_struct *p, int options, sig->oublock + sig->coublock; task_io_accounting_add(&psig->ioac, &p->ioac); task_io_accounting_add(&psig->ioac, &sig->ioac); + if (psig->cmaxrss < sig->maxrss) + psig->cmaxrss = sig->maxrss; spin_unlock_irq(&p->parent->sighand->siglock); } diff --git a/kernel/fork.c b/kernel/fork.c index 495da2e..36ac3e5 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -850,6 +850,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk) sig->min_flt = sig->maj_flt = sig->cmin_flt = sig->cmaj_flt = 0; sig->inblock = sig->oublock = sig->cinblock = sig->coublock = 0; task_io_accounting_init(&sig->ioac); + sig->maxrss = sig->cmaxrss = 0; taskstats_tgid_init(sig); task_lock(current->group_leader); diff --git a/kernel/sys.c b/kernel/sys.c index 31deba8..0441975 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1569,6 +1569,7 @@ static void k_getrusage(struct task_struct *p, int who, struct rusage *r) r->ru_majflt = p->signal->cmaj_flt; r->ru_inblock = p->signal->cinblock; r->ru_oublock = p->signal->coublock; + r->ru_maxrss = p->signal->cmaxrss; if (who == RUSAGE_CHILDREN) break; @@ -1583,6 +1584,8 @@ static void k_getrusage(struct task_struct *p, int who, struct rusage *r) r->ru_majflt += p->signal->maj_flt; r->ru_inblock += p->signal->inblock; r->ru_oublock += p->signal->oublock; + if (r->ru_maxrss < p->signal->maxrss) + r->ru_maxrss = p->signal->maxrss; t = p; do { accumulate_thread_rusage(t, r); @@ -1598,6 +1601,17 @@ static void k_getrusage(struct task_struct *p, int who, struct rusage *r) out: cputime_to_timeval(utime, &r->ru_utime); cputime_to_timeval(stime, &r->ru_stime); + + if (who != RUSAGE_CHILDREN) { + task_lock(p); + if (p->mm) { + unsigned long maxrss = get_mm_hiwater_rss(p->mm); + + if (r->ru_maxrss < maxrss) + r->ru_maxrss = maxrss; + } + task_unlock(p); + } } int getrusage(struct task_struct *p, int who, struct rusage __user *ru) -- 1.6.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <20081217105639.63e765eb-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org>]
* Re: [PATCH, RESEND2] getrusage: fill ru_maxrss value [not found] ` <20081217105639.63e765eb-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> @ 2008-12-17 10:48 ` KOSAKI Motohiro [not found] ` <20081217194646.F677.KOSAKI.MOTOHIRO-+CUm20s59erQFUHtdCDX3A@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: KOSAKI Motohiro @ 2008-12-17 10:48 UTC (permalink / raw) To: Jiri Pirko Cc: kosaki.motohiro-+CUm20s59erQFUHtdCDX3A, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api-u79uwXL29TY76Z2rM5mHXA, Hugh Dickins, jlim-sJ/iWh9BUns > (updated) > > This patch makes ->ru_maxrss value in struct rusage filled accordingly to > rss hiwater mark. This struct is filled as a parameter to > getrusage syscall. ->ru_maxrss value is set to pages which might be correct > as "time" application converts it to KBs. Why? if kernel convert to KB, glibc don't need any change. -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20081217194646.F677.KOSAKI.MOTOHIRO-+CUm20s59erQFUHtdCDX3A@public.gmane.org>]
* Re: [PATCH, RESEND2] getrusage: fill ru_maxrss value [not found] ` <20081217194646.F677.KOSAKI.MOTOHIRO-+CUm20s59erQFUHtdCDX3A@public.gmane.org> @ 2008-12-17 11:42 ` Jiri Pirko [not found] ` <20081217124256.46883fae-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2008-12-17 11:42 UTC (permalink / raw) Cc: kosaki.motohiro-+CUm20s59erQFUHtdCDX3A, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api-u79uwXL29TY76Z2rM5mHXA, Hugh Dickins, jlim-sJ/iWh9BUns On Wed, 17 Dec 2008 19:48:44 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote: > > (updated) > > > > This patch makes ->ru_maxrss value in struct rusage filled accordingly to > > rss hiwater mark. This struct is filled as a parameter to > > getrusage syscall. ->ru_maxrss value is set to pages which might be correct > > as "time" application converts it to KBs. > > Why? > if kernel convert to KB, glibc don't need any change. Where exactly glibc is working with this as KBs? I can't find that place. I looked into sources of time util and maxrss is showed this way: fprintf (fp, "%lu", ptok ((UL) resp->ru.ru_maxrss)); ptok() actually does pages_to_KB conversion. If we convert to KB in kernel, this code must be changed. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20081217124256.46883fae-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org>]
* Re: [PATCH, RESEND2] getrusage: fill ru_maxrss value [not found] ` <20081217124256.46883fae-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> @ 2008-12-17 12:03 ` KOSAKI Motohiro 2008-12-17 14:52 ` Jiri Pirko 0 siblings, 1 reply; 6+ messages in thread From: KOSAKI Motohiro @ 2008-12-17 12:03 UTC (permalink / raw) To: Jiri Pirko Cc: kosaki.motohiro-+CUm20s59erQFUHtdCDX3A, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api-u79uwXL29TY76Z2rM5mHXA, Hugh Dickins, jlim-sJ/iWh9BUns > > > This patch makes ->ru_maxrss value in struct rusage filled accordingly to > > > rss hiwater mark. This struct is filled as a parameter to > > > getrusage syscall. ->ru_maxrss value is set to pages which might be correct > > > as "time" application converts it to KBs. > > > > Why? > > if kernel convert to KB, glibc don't need any change. > Where exactly glibc is working with this as KBs? I can't find that place. > > I looked into sources of time util and maxrss is showed this way: > fprintf (fp, "%lu", ptok ((UL) resp->ru.ru_maxrss)); > ptok() actually does pages_to_KB conversion. If we convert to KB in > kernel, this code must be changed. Ah, you talked about /usr/bin/time? sorry, I misunderstood a bit. Why time need number of pages? In general, getrusage()::ru_maxrss is bsd compatibility feature. as far as possible, the same syscall spec is better. and bsd use KB unit. if time command has reasonable reason, I can agree current design. but is there? -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RESEND2] getrusage: fill ru_maxrss value 2008-12-17 12:03 ` KOSAKI Motohiro @ 2008-12-17 14:52 ` Jiri Pirko 2008-12-18 2:50 ` KOSAKI Motohiro 0 siblings, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2008-12-17 14:52 UTC (permalink / raw) Cc: kosaki.motohiro, linux-kernel, Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api, Hugh Dickins, jlim On Wed, 17 Dec 2008 21:03:27 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote: > > > > This patch makes ->ru_maxrss value in struct rusage filled accordingly to > > > > rss hiwater mark. This struct is filled as a parameter to > > > > getrusage syscall. ->ru_maxrss value is set to pages which might be correct > > > > as "time" application converts it to KBs. > > > > > > Why? > > > if kernel convert to KB, glibc don't need any change. > > Where exactly glibc is working with this as KBs? I can't find that place. > > > > I looked into sources of time util and maxrss is showed this way: > > fprintf (fp, "%lu", ptok ((UL) resp->ru.ru_maxrss)); > > ptok() actually does pages_to_KB conversion. If we convert to KB in > > kernel, this code must be changed. > > Ah, you talked about /usr/bin/time? sorry, I misunderstood a bit. > Why time need number of pages? > > In general, getrusage()::ru_maxrss is bsd compatibility feature. > as far as possible, the same syscall spec is better. > and bsd use KB unit. Oh you are right. Now I searched it in FreeBSD kernel. They goes like this: rss = pgtok(vmspace_resident_count(vm)); if (ru->ru_maxrss < rss) ru->ru_maxrss = rss; Seems pretty reasonable to stick with the same behavior. Then I really do not understand why /usr/bin/time does the conversion. FreeBSD /usr/bin/time is very different and much simpler and (of course) does not do this conversion. So I suggest to change the patch to fill KB instead of pages and change /usr/bin/time to not do the conversion. What do you think? > > if time command has reasonable reason, I can agree current design. > but is there? > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, RESEND2] getrusage: fill ru_maxrss value 2008-12-17 14:52 ` Jiri Pirko @ 2008-12-18 2:50 ` KOSAKI Motohiro 0 siblings, 0 replies; 6+ messages in thread From: KOSAKI Motohiro @ 2008-12-18 2:50 UTC (permalink / raw) To: Jiri Pirko Cc: kosaki.motohiro, linux-kernel, Oleg Nesterov, Andrew Morton, Michael Kerrisk, linux-api, Hugh Dickins, jlim > > Ah, you talked about /usr/bin/time? sorry, I misunderstood a bit. > > Why time need number of pages? > > > > In general, getrusage()::ru_maxrss is bsd compatibility feature. > > as far as possible, the same syscall spec is better. > > and bsd use KB unit. > Oh you are right. Now I searched it in FreeBSD kernel. They goes like this: > rss = pgtok(vmspace_resident_count(vm)); > if (ru->ru_maxrss < rss) > ru->ru_maxrss = rss; > > Seems pretty reasonable to stick with the same behavior. Then I really > do not understand why /usr/bin/time does the conversion. me too ;-) > FreeBSD /usr/bin/time is very different and much simpler and (of > course) does not do this conversion. > > So I suggest to change the patch to fill KB instead of pages and > change /usr/bin/time to not do the conversion. What do you think? Makes really much sense. thanks!! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-18 2:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-12-17 9:56 [PATCH, RESEND2] getrusage: fill ru_maxrss value Jiri Pirko [not found] ` <20081217105639.63e765eb-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> 2008-12-17 10:48 ` KOSAKI Motohiro [not found] ` <20081217194646.F677.KOSAKI.MOTOHIRO-+CUm20s59erQFUHtdCDX3A@public.gmane.org> 2008-12-17 11:42 ` Jiri Pirko [not found] ` <20081217124256.46883fae-YzwxZg+R7evSU73v1vjTzyO4YDw3rz4rAInAS/Ez/D0@public.gmane.org> 2008-12-17 12:03 ` KOSAKI Motohiro 2008-12-17 14:52 ` Jiri Pirko 2008-12-18 2:50 ` KOSAKI Motohiro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).