[ltt-dev] [PATCH v2] 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:
> >> 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 marker mutex is also used in marker_update_probe_range(), called
upon module load/unload, to insure consistency of the module sections.
So it's needed everywhere where the marker sections are read.

Mathieu

> > 
> > The same should be done for build_marker_control_files() and
> > remove_marker_control_dir().
> 
> -- 
> Regards
> Gui Jianfeng
> 

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