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

[Gui Jianfeng]

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?

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

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

-- 
Regards
Gui Jianfeng