From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21022 invoked by alias); 7 Oct 2009 13:07:42 -0000 Received: (qmail 21012 invoked by uid 22791); 7 Oct 2009 13:07:40 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=BAYES_00 X-Spam-Check-By: sourceware.org Received: from e23smtp05.au.ibm.com (HELO e23smtp05.au.ibm.com) (202.81.31.147) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Oct 2009 13:07:36 +0000 Received: from d23relay02.au.ibm.com (d23relay02.au.ibm.com [202.81.31.244]) by e23smtp05.au.ibm.com (8.14.3/8.13.1) with ESMTP id n97D4lSs006627 for ; Thu, 8 Oct 2009 00:04:47 +1100 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay02.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id n97D7VHT1155082 for ; Thu, 8 Oct 2009 00:07:31 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n97D7UEQ013210 for ; Thu, 8 Oct 2009 00:07:31 +1100 Received: from localhost (rajduddu.in.ibm.com [9.124.33.91]) by d23av04.au.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id n97D7TbF013188; Thu, 8 Oct 2009 00:07:30 +1100 Date: Wed, 07 Oct 2009 13:07:00 -0000 From: Rajasekhar Duddu To: "Frank Ch. Eigler" Cc: systemtap@sources.redhat.com Subject: Re: [PATCH v3] Tracepoint Tapset for Memory Subsystem Message-ID: <20091007130728.GA6574@rajduddu> References: <20090919050102.GA3767@rajduddu> <4AB90BE0.4030405@redhat.com> <4AB94A1B.4090801@redhat.com> <20090924180817.GA9698@rajduddu> <4ABD3B2B.4020107@redhat.com> <20090930101156.GA3792@rajduddu> <20091002151344.GA9516@rajduddu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2009-q4/txt/msg00077.txt.bz2 Hi Frank, thanks for your comments. I am answering for your questions here and in my next patch I will apply all the possible changes. On Tue, Oct 06, 2009 at 03:01:09PM -0400, Frank Ch. Eigler wrote: > Rajasekhar Duddu writes: > > > [...] > > +/* Function to convert the GFP_FLAGS . */ > > + > > +function gfp_flag_str:string (gfp_flag:long) > > +%{ > > +int flags = (int)THIS->gfp_flag; > > +THIS->__retvalue[0] = '\0'; > > + > > +#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 > > Why no "|" before GFP_HIGH/GFP_WAIT? All the other Flags (except GFP_WAIT/GFP_HIGH) are composition of one or the other Flags, and the composition starts with either GFP_HIGH or GFP_WAIT, so there is no "|" before GFP_HIGH/GFP_WAIT. > Also, why no "__" before the stringified version? This I will apply in the next patch. > > > > +#ifdef __GFP_FS > > + if (flags & __GFP_FS) > > + strlcat (THIS->__retvalue, "|GFP_FS",MAXSTRINGLEN); > > +#endif > > (How about a macro to generate all these near-identical branches?) > > Sure I will have macro something like this: %{ #define FLAG_TEST(TYPE) if (flags & TYPE) { strlcat \ (THIS->__retvalue, #TYPE,MAXSTRINGLEN); } %} and I will use that in my gfp_flag_str as fallowing. #ifdef __GFP_HIGH FLAG_TEST(__GFP_HIGH); #endif > > +%} > > > > +/** > > + * probe vm.kmalloc - Fires when kmalloc is requested. > > + * @call_site: Address of the caller function. > > + * @caller_function: Name of the caller function. > > + * @bytes_req: Requested Bytes > > + * @bytes_alloc: Allocated Bytes > > + * @gfp_flags: type of kmemory to allocate > > + * @ptr: Pointer to the kmemory allocated > > + */ > > + > > +probe vm.kmalloc = kernel.trace("kmalloc") { > > + name = "kmalloc" > > + call_site = $call_site > > + caller_function = symname(call_site) > > + bytes_req = $bytes_req > > + bytes_alloc = $bytes_alloc > > + gfp_flags = gfp_flag_str($gfp_flags) > > + ptr = $ptr > > +} > > Nice. I thought that the raison d'etre for these aliases was to > abstract the presence or absence of tracepoints, so is there no > fallback kprobe available? Something like this: > Fallback kprobe is not available for other memory functions because the variables exported by them are will be modified. > > > +probe __vm.kfree.kp = kernel.function("kfree") { > > + name = "kfree" > > + call_site = "0" > > (Note though that this will fail type checking on a non-tracepoint > kernel -- have you tried it? -- it should be just 0 instead of "0".) > This also I will apply in my next patch. > > + caller_function = "unknown" > > + ptr = $x > > +} > > + > > +probe __vm.kfree.tp = kernel.trace("kfree") { > > + name = "kfree" > > + call_site = $call_site > > + caller_function = symname(call_site) > > + ptr = $ptr > > +} > > + > > +/** > > + * probe vm.kfree - Fires when kfree is requested. > > + * @call_site: Address of the caller function (displayed if available) > > + * @caller_function - Name of the caller function (displayed if available) > > + * @ptr: Pointer to the kmemory allocated which is returned by kmalloc > > + */ > > +probe vm.kfree = __vm.kfree.tp !, > > + __vm.kfree.kp > > +{} > > Right. > > > > +/** > > + * probe vm.kmalloc_node - Fires when kmalloc_node is requested. > > + * @call_site: Address of the caller function. > > + * @caller_function: Name of the caller function. > > + * @bytes_req: Requested Bytes > > + * @bytes_alloc: Allocated Bytes > > + * @gfp_flags: Type of kmemory to allocate > > + * @ptr: Pointer to the kmemory allocated > > + */ > > Please, no "" markup in there, it is not valid. > This I will cange in my next patch. > > > +probe vm.kmalloc_node = kernel.trace("kmalloc_node")? { > > [...] > > Why is this marked with "?"? Kmalloc_node will be called when "CONFIG_NUMA" is defined, so I have used "?" to make it an optional. > > > > --- a/testsuite/buildok/vm.tracepoints.stp 1969-12-31 19:00:00.000000000 -0500 > > +++ b/testsuite/buildok/vm.tracepoints.stp 2009-10-02 10:59:20.000000000 -0400 > > @@ -0,0 +1,32 @@ > > +#!/usr/bin/stp -up4 > > Other similar test cases just use > > #! stap -up4 > This also I will change in my next patch. > Thanks -- Rajasekhar Duddu (rajduddu@linux.vnet.ibm.com), Linux on System z - CSVT, IBM LTC, Bangalore.