From mboxrd@z Thu Jan  1 00:00:00 1970
From: compudj@krystal.dyndns.org (Mathieu Desnoyers)
Date: Thu, 5 Mar 2009 13:53:12 -0500
Subject: [ltt-dev] [PATCH] LTTNG: Make marker enabled in advance
In-Reply-To: <49AF255A.4040502@cn.fujitsu.com>
References: <49AC9C77.5050206@cn.fujitsu.com> <20090304171022.GA2554@Krystal>
	<49AF255A.4040502@cn.fujitsu.com>
Message-ID: <20090305185312.GC6605@Krystal>

* Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> >> Hi Mathieu,
> >>
> >> This patch makes marker enabled in advance.
> >> IOW, even if the marker isn't present for the moment,
> >> you can still enable it for future use.
> >> As soon as the marker is inserted into kernel, tracing 
> >> work can be started immediately.
> >>
> >> Here is an example for using the user interface:
> >> This patch assumes the marker control patch is applied.
> >>
> >> [root at localhost markers]# cd /mnt/debugfs/ltt/markers
> >> [root at localhost markers]# mkdir fs
> >> [root at localhost markers]# cd fs
> >> [root at localhost fs]# mkdir close
> >> [root at localhost fs]# cd close/
> >> [root at localhost close]# ls
> >> enable  info
> >> [root at localhost close]# echo 1 > enable
> >> [root at localhost close]# cat enable
> >> 2
> >> [root at localhost close]# cat info
> >> marker is not present now!
> >> [root at localhost close]# modprobe fs_trace
> >> [root at localhost close]# cat enable
> >> 1
> >> [root at localhost close]# cat info
> >> format: "fd %u"
> >> state: 1
> >> event_id: 1
> >> call: 0xc0468167
> >> probe single : 0xc0520cdc
> >>
> >> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujitsu.com>
> >> ---
> >>  ltt/ltt-trace-control.c |  357 ++++++++++++++++++++++++++++++++---------------
> >>  1 files changed, 247 insertions(+), 110 deletions(-)
> >>
> >> diff --git a/ltt/ltt-trace-control.c b/ltt/ltt-trace-control.c
> >> index 128db4e..7e1c32e 100644
> >> --- a/ltt/ltt-trace-control.c
> >> +++ b/ltt/ltt-trace-control.c
> >> @@ -19,6 +19,7 @@
> >>  #include <linux/debugfs.h>
> >>  #include <linux/ltt-tracer.h>
> >>  #include <linux/notifier.h>
> >> +#include <linux/mutex.h>
> >>  
> >>  #define LTT_CONTROL_DIR "control"
> >>  #define MARKERS_CONTROL_DIR "markers"
> >> @@ -27,9 +28,240 @@
> >>  
> >>  #define LTT_WRITE_MAXLEN	(128)
> >>  
> >> +#define MARKER_ENABLE_MASK 0x7UL
> > 
> > What is this doing ?
> 
>   marker is 8 bytes aligned, the last 3 bit must 0.
>   I use these bits to identify whether the marker is enabled in advance.
> 

Is there a clear gain to encode such information in the pointer bits ?
Can we simply add a "char" somewhere which would be easier for the code
reviewers ? I rarely found out that this addition of complexity was
worth it, except when some updates need to be done atomically.

> > 
> >> +
> >> +static DEFINE_MUTEX(unload_mutex);
> > 
> > Can you tell me exactly what this protects, where it is nested ?
> 
>   when doing mkdir to create a new dir by debugfs_create_dir(), I have to drop
>   the parent's i_mutex, because debugfs_create_dir() need it. To prevent the 
>   parent to be removed, this mutex is introduced.
> 

So you want to protect against racy removal ? Which module is
responsible for such removal, and which mutex protects addition/removal?
I really doubt that anything near this unload_mutex is the right
solution.

Mathieu

> > 
> > Mathieu
> > 
> >> +
> 
> -- 
> Regards
> Gui Jianfeng
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68