From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Wed, 11 Feb 2009 00:32:21 -0500 Subject: [ltt-dev] [PATCH] LTTng: Make mark-control work in debugfs In-Reply-To: <498A3EF4.8080602@cn.fujitsu.com> References: <4988F393.8080605@cn.fujitsu.com> <20090204062051.GB10772@Krystal> <49893957.20303@cn.fujitsu.com> <20090204071255.GA12864@Krystal> <498A3EF4.8080602@cn.fujitsu.com> Message-ID: <20090211053221.GD8132@Krystal> * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote: > 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? > That's expected when a marker is a trace_mark_tp. That's the simplest way I found to deal with references. As I recall, a trace_mark() (not _tp) should not have this side-effect. So basically, it's mostly the lttng probes which have this refcounting. > > > > 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? > See the above paragraph about explanation around trace_mark_tp(). So basically, I expect that lttng module containing trace_mark_tp() should not be unloadable when armed. > > > > 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. > Ah, this is possible. Then we have to make sure the core event IDs are never allocated dynamically. I am pulling your new interface for 2.6.29-rc4, we'll start from there. As you say, it provides the same functionnality as the old interface. We might want to test the cases I spotted. They might well be problems that were there in the previous interface as well. Thanks, Mathieu > > > > 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 > > Signed-off-by: Mathieu Desnoyers > > --- > > 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 > > + * Copyright 2008 - Zhaolei > > + * > > + * Copyright 2009 - Gui Jianfeng > > + * Make mark-control work in debugfs > > */ > > > > /* > > @@ -16,14 +18,17 @@ > > #include > > #include > > #include > > +#include > > > > #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 > > > > _______________________________________________ > ltt-dev mailing list > ltt-dev at lists.casi.polymtl.ca > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68