Re: proc memory statistics tapset

[Mark Wielaard <mjw at redhat dot com>]

Hi Josh,

On Mon, 2009-10-05 at 11:09 -0700, Josh Stone wrote:
> On 10/05/2009 08:06 AM, Mark Wielaard wrote:
> > I would like to add it as tapset/proc_mem.stp and put the documentation
> > together with the Memory Tapset in the Tapset Reference manual since I
> > think they are generally useful. But if people feel they should go into
> > the examples only, that is fine too.
> 
> I think this is a fine thing to have as a tapset.  Perhaps your short
> script using it on stap is also good in the examples to demonstrate its
> simple use.

Thanks for going over it. Lets add it and see if people can poke holes
in it when they use it. I'll look into adding it also as an example.

> What unsafe contexts don't have current?  That's what I tried to find
> experimentally with testsuite/systemtap.stress/current.exp, and so far
> it seems that current is always available and safe.

yeah, you are right, current is always there. Some of these checks come
from when I wanted to provide the same functionality for task !=
current. But that is just asking for trouble it seems. So now we
restrict to current, which seems more sensible.

>   "Task isn't coopted
> by a kernel thread" doesn't make sense to me either -- kernel threads
> have their own current task_struct, represented by that KTHREAD flag.

I am not 100% sure that is correct. Task flags is set the PT_KTHREAD in
INIT_TASK() before it is fully created. I even extended that check to:
current->flags & (_STP_PF_KTHREAD | PF_EXITING | PF_STARTING)
just to be fully paranoid we never query some task-mm struct that isn't
setup right. Feel free to proof me wrong in being that paranoid :)

> I'm not sure why the CONTEXT->regs check matters.  You don't need it to
> get any of the stats, so I'd say leave it to the user to decide whether
> it makes sense to use it with their probe.  For example, tracepoints
> have no regs, but the user might still want to sample memory usage on
> sched_switch.

Agreed, check removed. Doesn't really make sense. What I wanted to catch
was "real processes" (as opposed to kernel threads), but I think the
above check takes care of that.

> Personally, I would this to look more like "ls -sh", "du -h", etc. -- no
> 'b', use upper-case K/M/G, and instead of your 5120 limit, just write it
> as X.Y if X < 10.  Or if you really like the extra detail, maybe write
> it padded to 4 characters -- X.YY, XX.Y, and then XXX and XXXX up to 1023.

That is nicer. Changed to your last suggestion.

I checked this in, with a testcase as:

commit 47f025139d1c2e75781cdab40dc9195396133754
Author: Mark Wielaard <mjw@redhat.com>
Date:   Tue Oct 6 19:24:22 2009 +0200

    Add proc_mem tapset, functions to query memory usage of the current
process.
    
    * tapset/proc_mem.stp: New tapset.
    * testsuite/buildok/proc_mem.stp
    * doc/SystemTap_Tapset_Reference/tapsets.tmpl (memory_stp): Include
      tapset/proc_mem.stp.

Thanks,

Mark