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

[Gui Jianfeng]

Mathieu Desnoyers wrote:
> * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
>> 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]# echo 1 > enable
>> [root at localhost close]# cat enable
>> 2
>> [root at localhost close]# cat info
>> Marker Pre-enabled
>> [root at localhost close]# modprobe fs-trace
>> [root at localhost close]# cat enable
>> 1
>> [root at localhost close]# cat info
>> Location: fs_trace
>> format: "fd %u"
>> state: 1
>> event_id: 0
>> call: 0xc0468192
>> probe single : 0xc0520ed8
>>
>> You can also remove a marker directory by "rmdir" if there
>> is no actual marker backed and isn't enabled in advance. 
>> A channel directory can be remove if there are not any marker
>> directories in it.
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujitsu.com>
>> ---
>>  include/linux/marker.h  |    1 +
>>  kernel/marker.c         |   29 ++++++++
>>  ltt/ltt-trace-control.c |  168 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 197 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/marker.h b/include/linux/marker.h
>> index 2d78113..692805c 100644
>> --- a/include/linux/marker.h
>> +++ b/include/linux/marker.h
>> @@ -261,5 +261,6 @@ extern void marker_iter_reset(struct marker_iter *iter);
>>  extern int marker_get_iter_range(struct marker **marker, struct marker *begin,
>>  	struct marker *end);
>>  extern int is_marker_enabled(const char *channel, const char *name);
>> +extern int is_marker_present(const char *channel, const char *name);
>>  
>>  #endif
>> diff --git a/kernel/marker.c b/kernel/marker.c
>> index 7d11393..0c4d5c9 100644
>> --- a/kernel/marker.c
>> +++ b/kernel/marker.c
>> @@ -651,6 +651,35 @@ static void disable_marker(struct marker *elem)
>>  }
>>  
>>  /*
>> + * is_marker_present - Check if a marker is present in kernel
>> + * @channel: channel name
>> + * @name: marker name
>> + *
>> + * Returns 1 if the marker is present, 0 if not.
>> + */
>> +int is_marker_present(const char *channel, const char *name)
>> +{
>> +	int ret;
>> +	struct marker_iter iter;
>> +
>> +	ret = 0;
> 
> Shouldn't this take a markers_mutex ?

  Does this place really need to take markers_mutex?
  IMHO, This mutex is used to protect the hash table, am i right.
  
> 
>> +	marker_iter_reset(&iter);
>> +	marker_iter_start(&iter);
>> +	for (; iter.marker != NULL; marker_iter_next(&iter)) {
>> +		if (!strcmp(iter.marker->channel, channel) &&
>> +		    !strcmp(iter.marker->name, name)) {
>> +			ret = 1;
>> +			goto end;
>> +		}
>> +	}
>> +	marker_iter_stop(&iter);
>> +
> 
>> +end:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(is_marker_present);
>> +
>> +/*
>>   * is_marker_enabled - Check if a marker is enabled
>>   * @channel: channel name
>>   * @name: marker name
>> diff --git a/ltt/ltt-trace-control.c b/ltt/ltt-trace-control.c
>> index c1c44c6..050c49f 100644
>> --- a/ltt/ltt-trace-control.c
>> +++ b/ltt/ltt-trace-control.c
>> @@ -756,6 +756,12 @@ static const struct file_operations ltt_destroy_trace_operations = {
>>  	.write = destroy_trace_write,
>>  };
>>  
>> +static void init_marker_dir(struct dentry *dentry,
>> +			    const struct inode_operations *opt)
>> +{
>> +	dentry->d_inode->i_op = opt;
>> +}
>> +
>>  static ssize_t marker_enable_read(struct file *filp, char __user *ubuf,
>>  			    size_t cnt, loff_t *ppos)
>>  {
>> @@ -769,10 +775,15 @@ static ssize_t marker_enable_read(struct file *filp, char __user *ubuf,
>>  	len = 0;
>>  	buf = (char *)__get_free_page(GFP_KERNEL);
>>  
>> -	if (is_marker_enabled(channel, marker))
>> +	if (is_marker_enabled(channel, marker) &&
>> +	    is_marker_present(channel, marker))
>>  		len = snprintf(buf, PAGE_SIZE, "%d\n", 1);
>> +	else if (is_marker_enabled(channel, marker) &&
>> +		 !is_marker_present(channel, marker))
>> +		len = snprintf(buf, PAGE_SIZE, "%d\n", 2);
>>  	else
>>  		len = snprintf(buf, PAGE_SIZE, "%d\n", 0);
>> +
>>  	if (len >= PAGE_SIZE) {
>>  		len = PAGE_SIZE;
>>  		buf[PAGE_SIZE] = '\0';
>> @@ -846,6 +857,13 @@ static ssize_t marker_info_read(struct file *filp, char __user *ubuf,
>>  	len = 0;
>>  	buf = (char *)__get_free_page(GFP_KERNEL);
>>  
>> +	if (is_marker_enabled(channel, marker) &&
>> +	    !is_marker_present(channel, marker)) {
>> +		len += snprintf(buf + len, PAGE_SIZE - len,
>> +				"Marker Pre-enabled\n");
>> +		goto out;
>> +	}
>> +
> 
> Hrm, I did not notice it, but marker_info_read, as it is currently
> implemented in LTTng, should take lock_markers() before iterating on the
> markers. Can you provide a separate patch that fixes this ? (I don't
> want to work under your feet...)

  Here is same, does it really need?

> 
> The same should be done for build_marker_control_files() and
> remove_marker_control_dir().

-- 
Regards
Gui Jianfeng