Re: [PATCH] getrusage: fill ru_maxrss value

On Tue, Dec 2, 2008 at 11:05 AM, Oleg Nesterov [off-list ref] wrote:

On 12/02, Jiri Pirko wrote:

quoted

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

Thanks Oleg.

Jiri: linux-api@ should also be CCed on API changes; see
Documentation/SubmitChecklist and
http://thread.gmane.org/gmane.linux.ltp/5658

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.

quoted

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.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html
--
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