This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v2] Tracepoint Tapset for Memory Subsystem
On 09/24/2009 01:08 PM, Rajasekhar Duddu wrote:
>
> Hi, I have modified the patch according to the comments passed
> by Frank and David.
>
> Many thanks to Frank And David.
See comments 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-24 19:01:10.000000000 +0200
> +++ b/tapset/memory.stp 2009-09-24 19:13:30.000000000 +0200
> @@ -195,3 +195,205 @@ probe vm.brk = kernel.function("do_brk")
> probe vm.oom_kill = kernel.function("__oom_kill_task") {
> task = $p
> }
> +#Function to get the call_site for kprobe based kfree probe.
> +
> +function call_site:long ()
> +%{
> + int *ptr ;
> + ptr = __builtin_return_address(0);
> + THIS->__retvalue = *ptr;
> +%}
Sorry to keep finding more things, but...
I'm a bit worried about your use of '__builtin_return_address()' here.
Jim Keniston reported on it back in 2005 in the following message, but
there isn't much context.
<http://sourceware.org/ml/systemtap/2005-q2/msg00242.html>
Jim, can you remember some context here? Was the use of
'__builtin_return_address' considered good/bad/neutral? We don't seem
to use it anywhere else.
> +
> +
> +/*Function to convert the GFP_FLAGS .*/
> +function gfp_f:string (gfp_flag:long)
> +%{
> +int flags = (int)THIS->gfp_flag;
> +
> +
> +#ifdef __GFP_HIGH
> + if (flags & __GFP_HIGH)
> + strlcat (THIS->__retvalue, "GFP_HIGH",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_WAIT
> + if (flags & __GFP_WAIT)
> + strlcat (THIS->__retvalue, "GFP_WAIT",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_IO
> + if (flags & __GFP_IO)
> + strlcat (THIS->__retvalue, "|GFP_IO",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_FS
> + if (flags & __GFP_FS)
> + strlcat (THIS->__retvalue, "|GFP_FS",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_COLD
> + if (flags & __GFP_COLD)
> + strlcat (THIS->__retvalue, "|GFP_COLD",MAXSTRINGLEN);
> +#endif
> +#ifdef __GFP_NOWARN
> + if (flags & __GFP_NOWARN)
> + strlcat (THIS->__retvalue, "|GFP_NOWARN",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_REPEAT
> + if (flags & __GFP_REPEAT)
> + strlcat (THIS->__retvalue, "|GFP_REPEAT",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_NOFAIL
> + if (flags & __GFP_NOFAIL)
> + strlcat (THIS->__retvalue, "|GFP_NOFAIL",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_NORETRY
> + if (flags & __GFP_NORETRY)
> + strlcat (THIS->__retvalue, "|GFP_NORETRY",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_COMP
> + if (flags & __GFP_COMP)
> + strlcat (THIS->__retvalue, "|GFP_COMP",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_ZERO
> + if (flags & __GFP_ZERO)
> + strlcat (THIS->__retvalue, "|GFP_ZERO",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_NOMEMALLOC
> + if (flags & __GFP_NOMEMALLOC)
> + strlcat (THIS->__retvalue, "|GFP_NOMEMALLOC",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_HARDWALL
> + if (flags & __GFP_HARDWALL)
> + strlcat (THIS->__retvalue, "|GFP_HARDWALL",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_THISNODE
> + if (flags & __GFP_THISNODE)
> + strlcat (THIS->__retvalue, "|GFP_THISNODE",MAXSTRINGLEN);
> +#endif
> +
> +#ifdef __GFP_RECLAIMABLE
> + if (flags & __GFP_RECLAIMABLE)
> + strlcat (THIS->__retvalue, "|GFP_RECLAIMABLE",MAXSTRINGLEN);
> +#endif
> +
> +
> +#ifdef __GFP_NOTRACK
> + if (flags & __GFP_NOTRACK)
> + strlcat (THIS->__retvalue, "|GFP_NOTRACK",MAXSTRINGLEN);
> +#endif
> +%}
The above function looks good. However, since you are using strlcat
exclusively, I'd probably start with this:
THIS->__retvalue[0] = '\0';
I'm probably being paranoid here, but it can't hurt.
Here's another minor point. I'd probably call this function something
like '_gfp_flag_str' (the '_f' suffix doesn't mean much to me).
... stuff deleted ...
> +/**
> + * probe kmem.kfree - Fires when <command>kfree</comand> is requested.
> + * @call_site: Address of the function calling this kmemory function.
> + * @ptr: Pointer to the kmemory allocated which is returned by kmalloc
> + */
> +probe kmem.kfree.kp = kernel.function("kfree") {
> + name = "kfree"
> + call_site = symname(call_site())
> + ptr = $x
> +}
> +
> +probe kmem.kfree.tp = kernel.trace("kfree") {
> + name = "kfree"
> + call_site = symname($call_site)
> + ptr = $ptr
> +}
> +
> +probe kmem.kfree = kmem.kfree.tp !,
> + kmem.kfree.kp
> +{}
At first I didn't see the need for the 2 separate probes, but I got it.
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)