Messages in this thread | | | Date | Tue, 2 Dec 2008 17:05:07 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH] getrusage: fill ru_maxrss value |
| |
On 12/02, Jiri Pirko wrote: > > Based on the patch previously posted by Frank Mayhar: > http://kerneltrap.org/mailarchive/linux-kernel/2007/9/20/264772 > > Changed to do not panic and to set the value properly. Also made some > modifications as Oleg suggested. maxrss value is set to pages as "time" > application converts it co KBs. > > This patch enables "time" application to show maxresident value > correctly.
I believe the changelog could be a bit more descriptive ;) And I think the patch needs more CCs. (Hugh and Michael cc'ed).
What about exec? Should we preserve signal_struct->maxrss, or should we reset it? I don't know what is the right behaviour, but imho it should be consistent with task_mem()/xacct_add_tsk(). And since bprm_mm_init() does not copy ->hiwater_xxx, perhaps we should reset. Or we should change the exec_mmap() path to preserve ->hiwater_xxx, I dunno.
> Note that even without this patch applied there is a race between two > parallel update_hiwater_rss() callers. > > [...snip...] > @@ -1051,6 +1051,8 @@ NORET_TYPE void do_exit(long code) > if (tsk->mm) { > update_hiwater_rss(tsk->mm); > update_hiwater_vm(tsk->mm); > + > + tsk->signal->maxrss = tsk->mm->hiwater_rss;
Yes. But still it is not good the patch adds new racy calls (k_getrusage() too). And in fact I think this code in do_exit() should die, and we can update signal->maxrss in exit_mm().
Unless I missed something, the "if (tsk->mm) {}" code in do_exit() is not needed because xacct_add_tsk() is not right anyway. What we imho need is get_mm_hiwater_xxx(), can be also used by task_mm().
I'll send the patch a bit later, then we can tweak this patch.
Do you agree?
(just in case, I don't mean that mm/ uses update_hiwater_xxx() "safely", afaics try_to_unmap can race with unmap_region for example, but still).
Oleg.
| |