From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@polymtl.ca (Mathieu Desnoyers) Date: Mon, 23 Mar 2009 13:10:42 -0400 Subject: [ltt-dev] [PATCH v3] LTTNG: Make marker enabled in advance In-Reply-To: <49C0C05A.3090505@cn.fujitsu.com> References: <49C0C05A.3090505@cn.fujitsu.com> Message-ID: <20090323171042.GE24084@Krystal> Hi Gui, Some comments follow : * 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 > --- > include/linux/marker.h | 1 + > kernel/marker.c | 33 +++++++++ > ltt/ltt-trace-control.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 201 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 93ffa8e..6161bee 100644 > --- a/kernel/marker.c > +++ b/kernel/marker.c > @@ -653,6 +653,39 @@ static void disable_marker(struct marker *elem) > } > > /* > + * is_marker_enabled - Check if a marker is present in kernel enabled -> present > + * @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; > + > + lock_markers(); > + > + 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); > + > + unlock_markers(); > +end: > + return ret; > +} > +EXPORT_SYMBOL_GPL(is_marker_present); > + You should probably create _is_marker_enabled() and _is_marker_present() which would not take the lock_markers() / unlock_markers, and lock the marker mutex around those 2 calls to ensure the state will stay consistent. > +/* > * 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 63cbb29..e2ff774 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)) Hrm 2 things : 1 - you are calling "is_marker_enabled" and "is_marker_present" twice. You should only call them once and use the result twice. 2 - Those two calls should probably be protected by the marker mutex, because it would give a weird result if a module containing a marker is loaded between the is_marker_enabled and the is_marker_present call. > + 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'; > @@ -848,6 +859,13 @@ static ssize_t marker_info_read(struct file *filp, char __user *ubuf, > > lock_markers(); > > + if (is_marker_enabled(channel, marker) && > + !is_marker_present(channel, marker)) { > + len += snprintf(buf + len, PAGE_SIZE - len, > + "Marker Pre-enabled\n"); Same here for the mutex. Mathieu > + goto out; > + } > + > marker_iter_reset(&iter); > marker_iter_start(&iter); > for (; iter.marker != NULL; marker_iter_next(&iter)) { > @@ -876,6 +894,7 @@ static ssize_t marker_info_read(struct file *filp, char __user *ubuf, > > unlock_markers(); > > +out: > if (len >= PAGE_SIZE) { > len = PAGE_SIZE; > buf[PAGE_SIZE] = '\0'; > @@ -891,6 +910,149 @@ static const struct file_operations info_fops = { > .read = marker_info_read, > }; > > +static int marker_mkdir(struct inode *dir, struct dentry *dentry, int mode) > +{ > + struct dentry *marker_d, *enable_d, *info_d, *channel_d; > + int ret; > + > + ret = 0; > + channel_d = (struct dentry *)dir->i_private; > + mutex_unlock(&dir->i_mutex); > + > + marker_d = debugfs_create_dir(dentry->d_name.name, > + channel_d); > + if (IS_ERR(marker_d)) { > + ret = PTR_ERR(marker_d); > + goto out; > + } > + > + enable_d = debugfs_create_file("enable", 0644, marker_d, > + NULL, &enable_fops); > + if (IS_ERR(enable_d) || !enable_d) { > + printk(KERN_ERR > + "%s: create file of %s failed\n", > + __func__, "enable"); > + ret = -ENOMEM; > + goto remove_marker_dir; > + } > + > + info_d = debugfs_create_file("info", 0644, marker_d, > + NULL, &info_fops); > + if (IS_ERR(info_d) || !info_d) { > + printk(KERN_ERR > + "%s: create file of %s failed\n", > + __func__, "info"); > + ret = -ENOMEM; > + goto remove_enable_dir; > + } > + > + goto out; > + > +remove_enable_dir: > + debugfs_remove(enable_d); > +remove_marker_dir: > + debugfs_remove(marker_d); > +out: > + mutex_lock_nested(&dir->i_mutex, I_MUTEX_PARENT); > + return ret; > +} > + > +static int marker_rmdir(struct inode *dir, struct dentry *dentry) > +{ > + struct dentry *marker_d, *channel_d; > + const char *channel, *name; > + int ret; > + > + ret = 0; > + > + channel_d = (struct dentry *)dir->i_private; > + channel = channel_d->d_name.name; > + > + marker_d = dir_lookup(channel_d, dentry->d_name.name); > + > + if (!marker_d) { > + ret = -ENOENT; > + goto out; > + } > + > + name = marker_d->d_name.name; > + > + if (is_marker_present(channel, name) || > + (!is_marker_present(channel, name) && > + is_marker_enabled(channel, name))) { > + ret = -EPERM; > + goto out; > + } > + > + mutex_unlock(&dir->i_mutex); > + mutex_unlock(&dentry->d_inode->i_mutex); > + debugfs_remove_recursive(marker_d); > + mutex_lock_nested(&dir->i_mutex, I_MUTEX_PARENT); > + mutex_lock(&dentry->d_inode->i_mutex); > +out: > + return ret; > +} > + > +const struct inode_operations channel_dir_opt = { > + .lookup = simple_lookup, > + .mkdir = marker_mkdir, > + .rmdir = marker_rmdir, > +}; > + > +static int channel_mkdir(struct inode *dir, struct dentry *dentry, int mode) > +{ > + struct dentry *channel_d; > + int ret; > + > + ret = 0; > + mutex_unlock(&dir->i_mutex); > + > + channel_d = debugfs_create_dir(dentry->d_name.name, > + markers_control_dir); > + if (IS_ERR(channel_d)) { > + ret = PTR_ERR(channel_d); > + goto out; > + } > + > + channel_d->d_inode->i_private = (void *)channel_d; > + init_marker_dir(channel_d, &channel_dir_opt); > +out: > + mutex_lock_nested(&dir->i_mutex, I_MUTEX_PARENT); > + return ret; > +} > + > +static int channel_rmdir(struct inode *dir, struct dentry *dentry) > +{ > + struct dentry *channel_d; > + int ret; > + > + ret = 0; > + > + channel_d = dir_lookup(markers_control_dir, dentry->d_name.name); > + if (!channel_d) { > + ret = -ENOENT; > + goto out; > + } > + > + if (list_empty(&channel_d->d_subdirs)) { > + mutex_unlock(&dir->i_mutex); > + mutex_unlock(&dentry->d_inode->i_mutex); > + debugfs_remove(channel_d); > + mutex_lock_nested(&dir->i_mutex, I_MUTEX_PARENT); > + mutex_lock(&dentry->d_inode->i_mutex); > + } else > + ret = -EPERM; > + > +out: > + return ret; > +} > + > +const struct inode_operations root_dir_opt = { > + .lookup = simple_lookup, > + .mkdir = channel_mkdir, > + .rmdir = channel_rmdir > +}; > + > static int build_marker_file(struct marker *marker) > { > struct dentry *channel_d, *marker_d, *enable_d, *info_d; > @@ -907,6 +1069,8 @@ static int build_marker_file(struct marker *marker) > err = -ENOMEM; > goto err_build_fail; > } > + channel_d->d_inode->i_private = (void *)channel_d; > + init_marker_dir(channel_d, &channel_dir_opt); > } > > marker_d = dir_lookup(channel_d, marker->name); > @@ -1130,6 +1294,8 @@ static int __init ltt_trace_control_init(void) > goto err_create_marker_control_dir; > } > > + init_marker_dir(markers_control_dir, &root_dir_opt); > + > if (build_marker_control_files()) > goto err_build_fail; > > -- > 1.5.4.rc3 > > > > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68