From mboxrd@z Thu Jan 1 00:00:00 1970 From: guijianfeng@cn.fujitsu.com (Gui Jianfeng) Date: Mon, 09 Mar 2009 09:23:53 +0800 Subject: [ltt-dev] [PATCH] LTTNG: Make marker enabled in advance In-Reply-To: <20090306182151.GE14236@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> <49B0C578.2010109@cn.fujitsu.com> <20090306182151.GE14236@Krystal> Message-ID: <49B46FA9.3090204@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: >>>>>> 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. > > Hi Gui, > >> 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. > > How often to you need the marker info ? > >> 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. >> > > You are optimizing a slow path. This is non-sense. Please use a solution > that is clean and works (and implies a hash table lookup) rather than to > do caches that will have no observable performance effects. And I cannot > stress enough the fact that the current situation is broken and will > leave marker modules unavailable in some module load/unload sequence. > We do not care about performance for marker connection/disconnection, > because this operation is rarely done, but we care a great deal about > correctness. OK, Will change. Thanks for your comments. ;) > > -- Regards Gui Jianfeng