Re: proc memory statistics tapset

[Josh Stone <jistone at redhat dot com>]

On 10/05/2009 08:06 AM, Mark Wielaard wrote:
> Hi,
> 
> I recently added printing of memory usage to each stap pass in verbose
> mode to see how much (extra) memory each pass contributed. Which helped
> me to keep track of memory usage while I refactored some of the dwfl
> construction code.
> 
> It occurred to me this is actually what we need to use systemtap for. We
> already have markers in stap itself that can be probed for the start and
> end of each pass. So instead of adding new source code I should have
> added a tapset that provides that information. That way it could be
> reused by anybody that wants to trace a program and get memory
> measurements/statistics. So that is what I just did.
> 
> [...]
> 
> 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.

>   /* Returns the mm for the current proc. Slightly paranoid. Only returns
>      from safe contexts (current must exist), and the task doesn't happens
>      to be (coopted by) a kernel thread. Callers also check CONTEXT->regs. */
>   static struct mm_struct *_stp_proc_mm(void)
>   {
>     struct task_struct *pid_task;
>     struct mm_struct *mm;
>     if (! current)
>       return NULL;
>     if (current->flags & _STP_PF_KTHREAD)
>       return NULL;
>     return current->mm;
>   }

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.  "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'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.

> /**
>  * sfunction bytes_to_string - Human readable string for given bytes.
>  *
>  * Description: Returns a string representing the number of bytes
>  * (when less than 5120b) postfixed by 'b', the number of kilobytes
>  * (when less than 5120k) postfixed by 'k', the number of megabytes
>  * (when less than 5120m) postfixed by 'm' or the number of gigabytes
>  * postfixed by 'g'.
>  */

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.

Josh