From mboxrd@z Thu Jan 1 00:00:00 1970 From: guijianfeng@cn.fujitsu.com (Gui Jianfeng) Date: Fri, 06 Mar 2009 14:40:56 +0800 Subject: [ltt-dev] [PATCH] LTTNG: Make marker enabled in advance In-Reply-To: <20090306033733.GA24899@Krystal> References: <49AC9C77.5050206@cn.fujitsu.com> <20090304171022.GA2554@Krystal> <49AF255A.4040502@cn.fujitsu.com> <20090305185312.GC6605@Krystal> <49B09139.6020904@cn.fujitsu.com> <20090306033733.GA24899@Krystal> Message-ID: <49B0C578.2010109@cn.fujitsu.com> Mathieu Desnoyers wrote: > * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote: >> Mathieu Desnoyers wrote: >>> * 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 >>>>>> --- >>>>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> #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. >> Hi Mathieu, >> >> I make use of inode->i_private of the file "enable" to store the corresponding >> marker address. If marker isn't present in kernel, but someone want to enable >> it in advance, inode->i_private will be 0x00000001. We don't need additional >> space to store *Pre-enable* status. >> If you have a better solution, could you give me some suggestions? >> > > Oh ! I see, you keep a pointer to struct marker as i_private of the > marker files in the debugfs tree. This is buggy, here is why : > > If the follow happens : > > module A has marker1 > module B has marker1 (too) > > We have : > > insmod module A > -> marker1 debugfs files created, pointing to module A marker1 > insmod module B > -> marker1 already exists, no update done, but "marker1" i_private > still points to module A's marker1 > rmmod module A > -> removing marker1, but module B still has one which should show up > in the directory tree. marker1 will not show up any more, it will gone when removing module A. So this won't cause serious problem. but this is a problem for sure, beacuse current implementation can't handle same name marker.... > > How to fix this ? Simply by _not_ keeping a direct reference to > struct marker within ltt-trace-control.c. > > We have a hash table in kernel/marker.c which takes care of exactly > this: it keeps a reference could of each marker loaded. Therefore, if > you need to get information about the state of such marker, you should > really use primitives like include/linux/marker.h:is_marker_enabled(), > and create your own primitives to export the information you need from > marker.c. > > This will solve you "is enabled" problem, because this hash table keeps > track of _activated_ markers. Therefore, for each marker you encounter > in the marker section (what you are iterating on upon module load > notification), you just have to query the is_marker_enabled() state for > the channel and marker name to get its state. > > You will however need to be called also when the marker state changes. > One way to do this would be to add a marker update notifier chain to > kernel/marker.c which would call your notifier callback. > > Looking at the register_module_notifier() use you are doing in > ltt_trace_control makes think of the following : kernel/marker.c should > use register_module_notifier(), but your module should not be called by > module.c. It should rather be called by marker.c, which is the module > which modifies the data structures your are interested to follow. > > Therefore, once you do that, keeping track of the marker state changes > will become _much_ easier, and you won't need any of these weird pointer > handling stuff. buffering a marker pointer can save search-time. IOW, when read "info" or "enable" to query information for a given marker, it has to iterate all the marker section in kernel and each modules to find out the marker. and this also save the time for calling is_marker_enabled() to search the hash table. My solution to solve the "same name marker" problem is that buffering the marker pointer in i_private if a marker doesn't have another same name marker. If there are two same name markers available, the 2nd lowest bit of i_private will indicate this situation and we need to iterate all the markers in kernel and modules to find out the corresponding markers. > >>>>>> + >>>>>> +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. >> Suppose we are creating a new channel dir, meanwhile we remove the ltt-trace-control >> module, all debugfs file will be removed. but debugfs_create_dir() is still in >> processing, but the ltt/marker is gone. Will this scenario happen? >> > > Hrm, upon entry in > > fs/namei.c: mkdirat, we have : > > dentry = lookup_create(&nd, 1); > > Which looks up for the parent path. This will hold the dentry counter on > the parent. It will execute the vfs_mkdir with this reference count > held. Therefore, the mkdir functions you created are called with the > parent's dentry refcount held, is that enough to insure the kind of > protection you want ? Is it possible that all other files are remove except the one we protects and new created files. If this can happen, i don't think it's ok. The goal of unload_mutex is protecting the whole tree. > > Mathieu > > >>> Mathieu >>> >>>>> Mathieu >>>>> >>>>>> + >>>> -- >>>> Regards >>>> Gui Jianfeng >>>> >> -- >> Regards >> Gui Jianfeng >> > -- Regards Gui Jianfeng