Message-ID: <Pine.LNX.4.64.0904051736210.23536@blonde.anvils>
Date:	Sun, 5 Apr 2009 18:24:22 +0100 (BST)
From:	Hugh Dickins <hugh@...itas.com>
To:	Jiri Pirko <jpirko@...hat.com>
cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	kosaki.motohiro@...fujitsu.com, oleg@...hat.com,
	linux-mm@...ck.org, mingo@...e.hu
Subject: Re: [PATCH for -mm] getrusage: fill ru_maxrss value

On Sun, 5 Apr 2009, Jiri Pirko wrote:
> (resend, repetitive patterns put into an inline function - not using max macro
>  because it's was decided not to use it in previous conversation)
> 
> From: Jiri Pirko <jpirko@...hat.com>
> 
> Make ->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 KBs which is the way it is done in BSD
> systems.  /usr/bin/time (gnu time) application converts ->ru_maxrss to KBs
> which seems to be incorrect behavior.  Maintainer of this util was
> notified by me with the patch which corrects it and cc'ed.
> 
> 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.
> 
> Note:
> exec() clear mm->hiwater_rss, but doesn't clear sig->maxrss.
> it is intetionally behavior. *BSD getrusage have exec() inheriting.

Sorry, I'm finding myself quite unable to Ack this: that may well
be a measure of my density, rather than a criticism of your patch.

On the nitpicking level: I wish you'd put the args the other way
round in your setmax_mm_hiwater_rss(maxrss, mm): (mm, maxrss) would
be more conventional.  Though we're gathering so many helpers for
these things, hard-to-distinguish without looking up in the header
file each time, that I wonder if they could all be refactored better.

But on the more serious level: I find I don't really understand what
this ru_maxrss number is supposed to be, so have a hard job telling
whether you've implemented it right (as to whether things are
updated in the right places, I'd rather rely on Oleg for that).

You convey the impression that it's supposed to be similar to
whatever it is in BSD, but little meaning beyond that.  We agreed
before that it should be derived from hiwater_rss - though now I
google for "BSD getrusage" I find ru_maxrss described as "maximum
shared memory or current resident set", which leaves me thoroughly
confused.

I'm worrying particularly about the fork/exec issue you highlight.
You're exemplary in providing your test programs, but there's a big
omission: you don't mention that the first test, "./getrusage -lc",
gives a very different result on Linux than you say it does on BSD -
you say the BSD fork line is "fork: self 0 children 0", whereas
I find my Linux fork line is "fork: self 102636 children 0".

So after that discrepancy, I can't tell what to expect.  Not that
I can make any sense of BSD's "self 0" there - I don't know how
you could present 0 there if this is related to hiwater_rss.

Now I'm seriously wondering if the ru_maxrss reported will generate
more bugreports from people puzzled as to how it should behave,
than help anyone in studying their process behaviour.

Sorry to be so negative after all this time: I genuinely hope others
will spring up to defend your patch and illustrate my stupidity.

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

Powered by blists - more mailing lists