[ltt-dev] [PATCH] LTTNG: same-name marker handling
Mathieu Desnoyers
mathieu.desnoyers at polymtl.ca
Tue Mar 10 12:14:17 EDT 2009
* Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> Hi Mathieu,
>
> This patch enables same-name marker handling in debugfs.
> The cached marker pointer is removed, and by looking up for
> each time instead.
> If there are two markers with the same channel name, marker name
> and format pair, for example, channel:fs, name:close, format:"fd %u"
> in fs_trace and and marker_example respectively. you can get the
> following message when you read "info" file in debugfs.
>
> [root at localhost close]# cat info
> Location: fs_trace
> format: "fd %u"
> state: 0
> event_id: 0
> call: 0xc0468167
> probe single : 0xc0466ed8
>
> Location: marker_example
> format: "fd %u"
> state: 0
> event_id: 0
> call: 0xc0468167
> probe single : 0xc0466ed8
>
> Location indicates where this marker locates.
>
> and enable them all by issuing the following command:
>
> [root at localhost close]# echo 1 > enable
>
> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujitsu.com>
Hi Gui,
It sounds globally good. Except for a few details, which I will fix
myself, but point out below for future reference :
> ---
> kernel/marker.c | 3 +-
> ltt/ltt-trace-control.c | 118 ++++++++++++++++++++++++++++++-----------------
> 2 files changed, 77 insertions(+), 44 deletions(-)
>
> diff --git a/kernel/marker.c b/kernel/marker.c
> index 3820622..bb2b718 100644
> --- a/kernel/marker.c
> +++ b/kernel/marker.c
> @@ -1321,6 +1321,7 @@ void exit_user_markers(struct task_struct *p)
> mutex_unlock(&markers_mutex);
> }
> }
> +#endif
>
> int is_marker_enabled(const char *channel, const char *name)
> {
> @@ -1332,7 +1333,7 @@ int is_marker_enabled(const char *channel, const char *name)
>
> return entry && !!entry->refcount;
> }
> -#endif
> +EXPORT_SYMBOL_GPL(is_marker_enabled);
>
is_marker_enabled was in the deprecated markers-userspace patch. I'll
have to reintroduce it in marker.c. I'll add it in the next version. You
should probably work on a more recent LTTng tree.
> int marker_module_notify(struct notifier_block *self,
> unsigned long val, void *data)
> diff --git a/ltt/ltt-trace-control.c b/ltt/ltt-trace-control.c
> index 128db4e..0dc5e93 100644
> --- a/ltt/ltt-trace-control.c
> +++ b/ltt/ltt-trace-control.c
> @@ -19,6 +19,7 @@
> #include <linux/debugfs.h>
> #include <linux/ltt-tracer.h>
> #include <linux/notifier.h>
> +#include <linux/marker.h>
>
> #define LTT_CONTROL_DIR "control"
> #define MARKERS_CONTROL_DIR "markers"
> @@ -60,7 +61,6 @@ static struct dentry *dir_lookup(struct dentry *parent, const char *name)
> return d;
> }
>
> -
Removing a whitespace is an unrelated patch is generally a bad idea.
> static ssize_t alloc_write(struct file *file, const char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> @@ -697,23 +697,24 @@ static struct file_operations ltt_destroy_trace_operations = {
> .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;
> +
Empty line here should be removed.
> char *buf;
> + const char *channel, *marker;
> int len;
>
> - marker = (struct marker *)filp->private_data;
> + marker = filp->f_dentry->d_parent->d_name.name;
> + channel = filp->f_dentry->d_parent->d_parent->d_name.name;
> +
> + len = 0;
> buf = kmalloc(1024, GFP_KERNEL);
>
> - len = sprintf(buf, "%d\n", _imv_read(marker->state));
> + if (is_marker_enabled(channel, marker))
> + len = sprintf(buf, "%d\n", 1);
> + else
> + len = sprintf(buf, "%d\n", 0);
>
> len = simple_read_from_buffer(ubuf, cnt, ppos, buf, len);
> kfree(buf);
> @@ -727,9 +728,11 @@ static ssize_t marker_enable_write(struct file *filp, const char __user *ubuf,
> char buf[NAME_MAX];
> int buf_size;
> int err = 0;
> - struct marker *marker;
> + const char *channel, *marker;
> +
> + marker = filp->f_dentry->d_parent->d_name.name;
> + channel = filp->f_dentry->d_parent->d_parent->d_name.name;
>
> - marker = (struct marker *)filp->private_data;
> buf_size = min(cnt, sizeof(buf) - 1);
> err = copy_from_user(buf, ubuf, buf_size);
> if (err)
> @@ -741,16 +744,14 @@ static ssize_t marker_enable_write(struct file *filp, const char __user *ubuf,
> case 'Y':
> case 'y':
> case '1':
> - err = ltt_marker_connect(marker->channel, marker->name,
> - "default");
> + err = ltt_marker_connect(channel, marker, "default");
> if (err)
> return err;
> break;
> case 'N':
> case 'n':
> case '0':
> - err = ltt_marker_disconnect(marker->channel, marker->name,
> - "default");
> + err = ltt_marker_disconnect(channel, marker, "default");
> if (err)
> return err;
> break;
> @@ -762,35 +763,46 @@ static ssize_t marker_enable_write(struct file *filp, const char __user *ubuf,
> }
>
> 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;
> + const char *channel, *marker;
> int len;
> + struct marker_iter iter;
> +
> + marker = filp->f_dentry->d_parent->d_name.name;
> + channel = filp->f_dentry->d_parent->d_parent->d_name.name;
>
> - marker = (struct marker *)filp->private_data;
> + len = 0;
> 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);
> + 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, marker))
> + len += sprintf(buf + len, "Location: %s\n"
Hrm, this bug was there previously, but is still here : you do not deal
with 1024 bytes overflow.
I'll also change the kmalloc(1024, ...); for a __get_free_page. This
will give us a 4kb tmp buffer without much more cost.
> + "format: \"%s\"\nstate: %d\n"
> + "event_id: %hu\n"
> + "call: 0x%p\n"
> + "probe %s : 0x%p\n\n",
> + iter.module ? iter.module->name :
> + "Core Kernel",
> + iter.marker->format,
> + _imv_read(iter.marker->state),
> + iter.marker->event_id,
> + iter.marker->call,
> + iter.marker->ptype ?
> + "multi" : "single", iter.marker->ptype ?
> + (void *)iter.marker->multi :
> + (void *)iter.marker->single.func);
> + }
> + marker_iter_stop(&iter);
>
> len = simple_read_from_buffer(ubuf, cnt, ppos, buf, len);
> kfree(buf);
> @@ -799,7 +811,6 @@ static ssize_t marker_info_read(struct file *filp, char __user *ubuf,
> }
>
> static const struct file_operations info_fops = {
> - .open = marker_info_open,
> .read = marker_info_read,
> };
>
> @@ -836,7 +847,7 @@ static int build_marker_file(struct marker *marker)
> enable_d = dir_lookup(marker_d, "enable");
> if (!enable_d) {
> enable_d = debugfs_create_file("enable", 0644, marker_d,
> - marker, &enable_fops);
> + NULL, &enable_fops);
> if (IS_ERR(enable_d) || !enable_d) {
> printk(KERN_ERR
> "%s: create file of %s failed\n",
> @@ -849,7 +860,7 @@ static int build_marker_file(struct marker *marker)
> info_d = dir_lookup(marker_d, "info");
> if (!info_d) {
> info_d = debugfs_create_file("info", 0444, marker_d,
> - marker, &info_fops);
> + NULL, &info_fops);
> if (IS_ERR(info_d) || !info_d) {
> printk(KERN_ERR
> "%s: create file of %s failed\n",
> @@ -888,26 +899,46 @@ err_build_fail:
> return err;
> }
>
> -static int remove_marker_control_dir(struct marker *marker)
> +static int remove_marker_control_dir(struct module *mod, struct marker *marker)
> {
> struct dentry *channel_d, *marker_d;
> + const char *channel, *name;
> + int count;
> + struct marker_iter iter;
> +
> + count = 0;
>
> channel_d = dir_lookup(markers_control_dir, marker->channel);
> if (!channel_d)
> return -ENOENT;
> + channel = channel_d->d_name.name;
>
> marker_d = dir_lookup(channel_d, marker->name);
> if (!marker_d)
> return -ENOENT;
> + name = marker_d->d_name.name;
> +
> + 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) && mod != iter.module)
> + count++;
OK, so this is without the mkdir feature ? Because with mkdir support,
we should probably keep the channel around even if there is no reference
left. Same for marker files. Either we check for "is marker enabled",
and if yes we keep it, or we add an extra reference count when mkdir is
done on the channel/marker, and only remove those when a rmdir is done
on them. I'll let you figure this out in the next patches for mkdir
support.
> + }
> +
> + if (count > 0)
> + goto end;
>
> debugfs_remove_recursive(marker_d);
> if (list_empty(&channel_d->d_subdirs))
> debugfs_remove(channel_d);
>
> +end:
missing :
marker_iter_stop(&iter);
I'll fix those and merge your patch in the lttng tree. Thanks !
Mathieu
> return 0;
> }
>
> -static void cleanup_control_dir(struct marker *begin, struct marker *end)
> +static void cleanup_control_dir(struct module *mod, struct marker *begin,
> + struct marker *end)
> {
> struct marker *iter;
>
> @@ -915,12 +946,13 @@ static void cleanup_control_dir(struct marker *begin, struct marker *end)
> return;
>
> for (iter = begin; iter < end; iter++)
> - remove_marker_control_dir(iter);
> + remove_marker_control_dir(mod, iter);
>
> return;
> }
>
> -static void build_control_dir(struct marker *begin, struct marker *end)
> +static void build_control_dir(struct module *mod, struct marker *begin,
> + struct marker *end)
> {
> struct marker *iter;
> int err;
> @@ -937,7 +969,7 @@ static void build_control_dir(struct marker *begin, struct marker *end)
>
> return;
> err_build_fail:
> - cleanup_control_dir(begin, end);
> + cleanup_control_dir(mod, begin, end);
> }
>
> static int module_notify(struct notifier_block *self,
> @@ -947,11 +979,11 @@ static int module_notify(struct notifier_block *self,
>
> switch (val) {
> case MODULE_STATE_COMING:
> - build_control_dir(mod->markers,
> + build_control_dir(mod, mod->markers,
> mod->markers + mod->num_markers);
> break;
> case MODULE_STATE_GOING:
> - cleanup_control_dir(mod->markers,
> + cleanup_control_dir(mod, mod->markers,
> mod->markers + mod->num_markers);
> break;
> }
> --
> 1.5.4.rc3
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
More information about the lttng-dev
mailing list