[ltt-dev] [PATCH] LTTNG: Make marker enabled in advance

[Mathieu Desnoyers]

* 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 <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.
> 
>   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.

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.

> > 
> >>>> +
> >>>> +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 ?

Mathieu


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

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