Re: [PATCH] Tracepoint Tapset for Memory Subsystem

[David Smith <dsmith@redhat.com>]

On 09/19/2009 12:01 AM, Rajasekhar Duddu wrote:
> Hi all,
> 	here I am posting a patch for Memory Tapset based on
> kernel Tracepoints. 
> 
> This patch adds tracepoint tapset to memory.stp and a testcase 
> (mem_tracepoints.stp) which tests if all probes in the
> tapset are resolvable .

In general this looks reasonable, and thanks for including the testcase.

One comment below.

> 
> 
> Signed-off-by: Rajasekhar Duddu <rajduddu@linux.vnet.ibm.com>
> diff -rupaN a/tapset/memory.stp b/tapset/memory.stp
> --- a/tapset/memory.stp	2009-09-16 14:24:21.000000000 +0200
> +++ b/tapset/memory.stp	2009-09-19 06:33:34.000000000 +0200
> @@ -195,3 +195,132 @@ probe vm.brk = kernel.function("do_brk")
>  probe vm.oom_kill = kernel.function("__oom_kill_task") {
>      task = $p
>  }
> +
> +
> +/*Function to convert the GFP_FLAGS .*/
> +function gfp_f:string (gfp_flag:long)
> +%{
> +        switch (THIS->gfp_flag) {
> +                case 32:
> +                        strlcpy(THIS->__retvalue, "GFP_ATOMIC", MAXSTRINGLEN);
> +                        break;
> +                case 208:
> +                        strlcpy(THIS->__retvalue, "GFP_KERNEL", MAXSTRINGLEN);
> +                        break;
> +                case 16:
> +                        strlcpy(THIS->__retvalue, "GFP_NOIO", MAXSTRINGLEN);
> +                        break;
> +                case 80:
> +                        strlcpy(THIS->__retvalue, "GFP_NOFS", MAXSTRINGLEN);
> +                        break;
> +                case 524496:
> +                        strlcpy(THIS->__retvalue, "GFP_TEMPORARY", MAXSTRINGLEN);
> +                        break;
> +                case 8400:
> +                        strlcpy(THIS->__retvalue, "GFP_USER", MAXSTRINGLEN);
> +                        break;
> +                case 8402:
> +                        strlcpy(THIS->__retvalue, "GFP_HIGHUSER", MAXSTRINGLEN);
> +                        break;
> +                case 8410:
> +                       strlcpy(THIS->__retvalue, "GFP_HIGHUSER_MOVABLE", MAXSTRINGLEN);
> +                        break;
> +                default:
> +                        break;
> +        }
> +
> +%}

Hmm.  Have you tried something like this instead (to avoid the hardcoded
constants)?  Note that I haven't actually tried this...

====
%{
#include <linux/gfp.h>
%}

function gfp_f:string (gfp_flag:long)
%{
    switch (THIS->gfp_flag) {
        case GFP_ATOMIC:
                strlcpy(THIS->__retvalue, "GFP_ATOMIC", MAXSTRINGLEN);
                break;
        case GFP_KERNEL:
                strlcpy(THIS->__retvalue, "GFP_KERNEL", MAXSTRINGLEN);
                break;
     ...
%}
====

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)