[ltt-dev] [PATCH] LTTng: Make mark-control work in debugfs
Mathieu Desnoyers
compudj at krystal.dyndns.org
Wed Feb 11 03:43:00 EST 2009
* Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
> * 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.
>
I think I have an idea we could implement to support arming markers
which are not loaded in the kernel yet. The use-case is :
We have a marker in a given module, which is not loaded. We are
interested in tracing events in this module happening soon after module
load. Therefore, we cannot use the /mnt/debugfs/ltt/markers/**/* files,
because the marker is not present yet.
We could implement mkdir operations to support this. This is out of
ltt-armall scope, and people would have to use it manually and *know*
what channel and marker names they want to enable, but I think it's ok.
So basically, we would have :
cd /mnt/debugfs/ltt
mkdir channel_name
cd channel_name
mkdir marker_name
The marker_name subdirectory would be populated with standard
information and enable files.
Do you think it could be added easily to ltt-trace-control ?
Mathieu
> 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 <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
> >
> >
> >
> > _______________________________________________
> > 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
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
More information about the lttng-dev
mailing list