From mboxrd@z Thu Jan  1 00:00:00 1970
From: guijianfeng@cn.fujitsu.com (Gui Jianfeng)
Date: Thu, 05 Feb 2009 09:20:52 +0800
Subject: [ltt-dev] [PATCH] LTTng: Make mark-control work in debugfs
In-Reply-To: <20090204071255.GA12864@Krystal>
References: <4988F393.8080605@cn.fujitsu.com> <20090204062051.GB10772@Krystal>
	<49893957.20303@cn.fujitsu.com> <20090204071255.GA12864@Krystal>
Message-ID: <498A3EF4.8080602@cn.fujitsu.com>

Mathieu Desnoyers wrote:
> 
> Hrm, I'm playing a bit with the interface, and I think I should wait
> before I merge it. There are a few things to fix.
> 
> First, when enabling a marker with the old ltt-marker-control.ko, if we
> rmmod the module containing this marker and after modprobe the module
> again, the marker stays enabled. With this debugfs interface, it seems
> to become disabled.

  Hi Mathieu,

  When enabling a marker with either way(old proc or debugfs), if this marker
  belongs a module, I'm afraid this module can't be unloaded any more.
  For example:
  #modprobe fs_trace
  #echo "connect fs close default" > /proc/ltt
  #rmmod fs_trace
  We'll get:
  ERROR: Module fs_trace is in use

  I tested under lttng-0.91, do i miss something?

> 
> Also, when I connect to a marker, there is some module refcounting
> involved to make sure the marked modules stays loaded. For some reason
> I've been able to unload a module with armed markers.. this should
> probably be looked at.

  Do you mean even if the marker is enabled(by using debugfs or proc interface),
  the module which includes this marker are still able to be unloaded?

> 
> Thirdly, when I do
> 
> cd /mnt/debugfs/ltt/markers
> for a in */*/enable; do echo $a; echo 1 > $a; done
> 
> I get :
> metadata/core_marker_format
> -su: echo: write error: File exists
> metadata/core_marker_id
> -su: echo: write error: File exists
> 
> Which is expected, because those marker are activated within the kernel
> and should not have their state modified by the user-visible API.
> 
> Then I do :
> for a in */*/enable; do echo $a; echo 0 > $a; done
> 
> And do the first command again :
> for a in */*/enable; do echo $a; echo 1 > $a; done
> 
> This shows that userspace is able to alter the state of builtin "core"
> markers, which I disallow in ltt-marker-control.ko. The same would be
> good in the debugfs interface.

  I tried on lttng-0.91
  #echo "disconnect metadata core_marker_format" > /proc/ltt
  #cat /proc/ltt | grep core_marker_format
  i got the following:
    channel: metadata marker: core_marker_format format: "channel %s name %s format %s" state: 0 event_id: 1 call: 0xc0460b7c probe single : 0xc045f904
  
  It seems core_marker_format has been disabled by old interface. Do i miss something again?

  ltt-armall do the restriction, consider the follwing piece of code 
  MARKERS=`cat /proc/ltt|grep -v %k|awk '{print $2 " " $4}'|sort -u|grep -v ^metadata
|          grep -v ^locking|grep -v ^lockdep`
  Actually, this block of code do the restriction from userspace.

> 
> I'm providing the patch modified to fit with checkpatch below.
> 
> Thanks,
> 
> Mathieu
> 
> 
> lttng-make-mark-control-work-in-debugfs
> 
> This patch makes mark-control work in debugfs
> Control files are organized as following.
> ltt/
> |-- markers
> |   |-- fs
> |   |   |-- buffer_wait_end
> |   |   |   |-- enable
> |   |   |   `-- info
> |   |   |-- buffer_wait_start
> |   |   |   |-- enable
> |   |   |   `-- info
> |   |   |-- close
> |   |   |   |-- enable
> |   |   |   `-- info
> |   |   |-- exec
> |   |   |   |-- enable
> |   |   |   `-- info
> ...
> 
> One can enable or disable a marker by using the following command
> # echo 1 > ltt/markers/fs/close/enable
> # echo 0 > ltt/markers/fs/close/enable
> 
> Show the marker information as following 
> # cat ltt/markers/fs/close/info
> format: "fd %u"
> state: 1
> event_id: 2
> call: 0xc0459118
> probe single : 0xc04f7b08
> 
> Mathieu :
> 
> enabled -> enable to follow the semantic of the rest of the debugfs API.
> checkpatch.pl cleanup.
> 
> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujitsu.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at polymtl.ca>
> ---
>  ltt/ltt-trace-control.c |  298 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 294 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6-lttng/ltt/ltt-trace-control.c
> ===================================================================
> --- linux-2.6-lttng.orig/ltt/ltt-trace-control.c	2009-02-04 00:56:05.000000000 -0500
> +++ linux-2.6-lttng/ltt/ltt-trace-control.c	2009-02-04 01:01:46.000000000 -0500
> @@ -1,8 +1,10 @@
>  /*
>   * LTT trace control module over debugfs.
>   *
> - * Copyright 2008 -
> - *	Zhaolei <zhaolei at cn.fujitsu.com>
> + * Copyright 2008 - Zhaolei <zhaolei at cn.fujitsu.com>
> + *
> + * Copyright 2009 - Gui Jianfeng <guijianfeng at cn.fujitsu.com>
> + *                  Make mark-control work in debugfs
>   */
>  
>  /*
> @@ -16,14 +18,17 @@
>  #include <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <linux/ltt-tracer.h>
> +#include <linux/notifier.h>
>  
>  #define LTT_CONTROL_DIR "control"
> +#define MARKERS_CONTROL_DIR "markers"
>  #define LTT_SETUP_TRACE_FILE "setup_trace"
>  #define LTT_DESTROY_TRACE_FILE "destroy_trace"
>  
>  #define LTT_WRITE_MAXLEN	(128)
>  
> -struct dentry *ltt_control_dir, *ltt_setup_trace_file, *ltt_destroy_trace_file;
> +struct dentry *ltt_control_dir, *ltt_setup_trace_file, *ltt_destroy_trace_file,
> +	*markers_control_dir;
>  
>  /*
>   * the traces_lock nests inside control_lock.
> @@ -692,6 +697,270 @@ static struct file_operations ltt_destro
>  	.write = destroy_trace_write,
>  };
>  
> +static int marker_enable_open(struct inode *inode, struct file *filp)
> +{
> +	filp->private_data = inode->i_private;
> +	return 0;
> +}
> +
> +static ssize_t marker_enable_read(struct file *filp, char __user *ubuf,
> +			    size_t cnt, loff_t *ppos)
> +{
> +	struct marker *marker;
> +	char *buf;
> +	int len;
> +
> +	marker = (struct marker *)filp->private_data;
> +	buf = kmalloc(1024, GFP_KERNEL);
> +
> +	len = sprintf(buf, "%d\n", _imv_read(marker->state));
> +
> +	len = simple_read_from_buffer(ubuf, cnt, ppos, buf, len);
> +	kfree(buf);
> +
> +	return len;
> +}
> +
> +static ssize_t marker_enable_write(struct file *filp, const char __user *ubuf,
> +				size_t cnt, loff_t *ppos)
> +{
> +	char buf[NAME_MAX];
> +	int buf_size;
> +	int err = 0;
> +	struct marker *marker;
> +
> +	marker = (struct marker *)filp->private_data;
> +	buf_size = min(cnt, sizeof(buf) - 1);
> +	err = copy_from_user(buf, ubuf, buf_size);
> +	if (err)
> +		return err;
> +
> +	buf[buf_size] = 0;
> +
> +	switch (buf[0]) {
> +	case 'Y':
> +	case 'y':
> +	case '1':
> +		err = ltt_marker_connect(marker->channel, marker->name,
> +					 "default");
> +		if (err)
> +			return err;
> +		break;
> +	case 'N':
> +	case 'n':
> +	case '0':
> +		err = ltt_marker_disconnect(marker->channel, marker->name,
> +					    "default");
> +		if (err)
> +			return err;
> +		break;
> +	default:
> +		return -EPERM;
> +	}
> +
> +	return cnt;
> +}
> +
> +static const struct file_operations enable_fops = {
> +	.open = marker_enable_open,
> +	.read = marker_enable_read,
> +	.write = marker_enable_write,
> +};
> +
> +static int marker_info_open(struct inode *inode, struct file *filp)
> +{
> +	filp->private_data = inode->i_private;
> +	return 0;
> +}
> +
> +static ssize_t marker_info_read(struct file *filp, char __user *ubuf,
> +			    size_t cnt, loff_t *ppos)
> +{
> +	struct marker *marker;
> +	char *buf;
> +	int len;
> +
> +	marker = (struct marker *)filp->private_data;
> +	buf = kmalloc(1024, GFP_KERNEL);
> +
> +	len = sprintf(buf, "format: \"%s\"\nstate: %d\n"
> +		      "event_id: %hu\n"
> +		      "call: 0x%p\n"
> +		      "probe %s : 0x%p\n",
> +		      marker->format, _imv_read(marker->state),
> +		      marker->event_id, marker->call, marker->ptype ?
> +		      "multi" : "single", marker->ptype ?
> +		      (void *)marker->multi : (void *)marker->single.func);
> +
> +	len = simple_read_from_buffer(ubuf, cnt, ppos, buf, len);
> +	kfree(buf);
> +
> +	return len;
> +}
> +
> +static const struct file_operations info_fops = {
> +	.open = marker_info_open,
> +	.read = marker_info_read,
> +};
> +
> +static int build_marker_file(struct marker *marker)
> +{
> +	struct dentry *channel_d, *marker_d, *enable_d, *info_d;
> +	int err;
> +
> +	channel_d = dir_lookup(markers_control_dir, marker->channel);
> +	if (!channel_d) {
> +		channel_d = debugfs_create_dir(marker->channel,
> +					       markers_control_dir);
> +		if (IS_ERR(channel_d) || !channel_d) {
> +			printk(KERN_ERR
> +			       "%s: build channel dir of %s failed\n",
> +			       __func__, marker->channel);
> +			err = -ENOMEM;
> +			goto err_build_fail;
> +		}
> +	}
> +
> +	marker_d  = dir_lookup(channel_d, marker->name);
> +	if (!marker_d) {
> +		marker_d = debugfs_create_dir(marker->name, channel_d);
> +		if (IS_ERR(marker_d) || !marker_d) {
> +			printk(KERN_ERR
> +			       "%s: marker dir of %s failed\n",
> +			       __func__, marker->name);
> +			err = -ENOMEM;
> +			goto err_build_fail;
> +		}
> +	}
> +
> +	enable_d = dir_lookup(marker_d, "enable");
> +	if (!enable_d) {
> +		enable_d = debugfs_create_file("enable", 0644, marker_d,
> +						marker, &enable_fops);
> +		if (IS_ERR(enable_d) || !enable_d) {
> +			printk(KERN_ERR
> +			       "%s: create file of %s failed\n",
> +			       __func__, "enable");
> +			err = -ENOMEM;
> +			goto err_build_fail;
> +		}
> +	}
> +
> +	info_d = dir_lookup(marker_d, "info");
> +	if (!info_d) {
> +		info_d = debugfs_create_file("info", 0444, marker_d,
> +						marker, &info_fops);
> +		if (IS_ERR(info_d) || !info_d) {
> +			printk(KERN_ERR
> +			       "%s: create file of %s failed\n",
> +			       __func__, "enable");
> +			err = -ENOMEM;
> +			goto err_build_fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_build_fail:
> +	return err;
> +}
> +
> +static int build_marker_control_files(void)
> +{
> +	struct marker_iter iter;
> +	int err;
> +
> +	err = 0;
> +	if (!markers_control_dir)
> +		return -EEXIST;
> +
> +	marker_iter_reset(&iter);
> +	marker_iter_start(&iter);
> +	for (; iter.marker != NULL; marker_iter_next(&iter)) {
> +		err = build_marker_file(iter.marker);
> +		if (err)
> +			goto err_build_fail;
> +	}
> +	marker_iter_stop(&iter);
> +	return 0;
> +
> +err_build_fail:
> +	return err;
> +}
> +
> +static int remove_marker_control_dir(struct marker *marker)
> +{
> +	struct dentry *channel_d, *marker_d;
> +
> +	channel_d = dir_lookup(markers_control_dir, marker->channel);
> +	if (!channel_d)
> +		return -ENOENT;
> +
> +	marker_d = dir_lookup(channel_d, marker->name);
> +	if (!marker_d)
> +		return -ENOENT;
> +
> +	debugfs_remove_recursive(marker_d);
> +	if (list_empty(&channel_d->d_subdirs))
> +		debugfs_remove(channel_d);
> +
> +	return 0;
> +}
> +
> +static void cleanup_control_dir(struct marker *begin, struct marker *end)
> +{
> +	struct marker *iter;
> +
> +	if (!markers_control_dir)
> +		return;
> +
> +	for (iter = begin; iter < end; iter++)
> +		remove_marker_control_dir(iter);
> +
> +	return;
> +}
> +
> +static void build_control_dir(struct marker *begin, struct marker *end)
> +{
> +	struct marker *iter;
> +	int err;
> +
> +	err = 0;
> +	if (!markers_control_dir)
> +		return;
> +
> +	for (iter = begin; iter < end; iter++) {
> +		err = build_marker_file(iter);
> +		if (err)
> +			goto err_build_fail;
> +	}
> +
> +	return;
> +err_build_fail:
> +	cleanup_control_dir(begin, end);
> +}
> +
> +static int module_notify(struct notifier_block *self,
> +		  unsigned long val, void *data)
> +{
> +	struct module *mod = data;
> +
> +	switch (val) {
> +	case MODULE_STATE_COMING:
> +		build_control_dir(mod->markers,
> +				  mod->markers + mod->num_markers);
> +		break;
> +	case MODULE_STATE_GOING:
> +		cleanup_control_dir(mod->markers,
> +				    mod->markers + mod->num_markers);
> +		break;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block module_nb = {
> +	.notifier_call = module_notify,
> +};
>  
>  static int __init ltt_trace_control_init(void)
>  {
> @@ -733,8 +1002,27 @@ static int __init ltt_trace_control_init
>  		goto err_create_destroy_trace_file;
>  	}
>  
> -	return 0;
> +	markers_control_dir = debugfs_create_dir(MARKERS_CONTROL_DIR,
> +						 ltt_root_dentry);
> +	if (IS_ERR(markers_control_dir) || !markers_control_dir) {
> +		printk(KERN_ERR
> +			"ltt_channel_control_init: create dir of %s failed\n",
> +			MARKERS_CONTROL_DIR);
> +		err = -ENOMEM;
> +		goto err_create_marker_control_dir;
> +	}
>  
> +	if (build_marker_control_files())
> +		goto err_build_fail;
> +
> +	if (!register_module_notifier(&module_nb))
> +		return 0;
> +
> +err_build_fail:
> +	debugfs_remove_recursive(markers_control_dir);
> +	markers_control_dir = NULL;
> +err_create_marker_control_dir:
> +	debugfs_remove(ltt_destroy_trace_file);
>  err_create_destroy_trace_file:
>  	debugfs_remove(ltt_setup_trace_file);
>  err_create_setup_trace_file:
> @@ -759,6 +1047,8 @@ static void __exit ltt_trace_control_exi
>  	debugfs_remove(ltt_setup_trace_file);
>  	debugfs_remove(ltt_destroy_trace_file);
>  	debugfs_remove_recursive(ltt_control_dir);
> +	debugfs_remove_recursive(markers_control_dir);
> +	unregister_module_notifier(&module_nb);
>  }
>  
>  module_init(ltt_trace_control_init);
> 
> 

-- 
Regards
Gui Jianfeng