[PATCH 1/2] tracing/ftrace: handle more than one stat file per tracer

[PATCH 1/2] tracing/ftrace: handle more than one stat file per tracer

[Frederic Weisbecker]

Impact: new API for tracers

Make the stat tracing API reentrant. And also provide the new directory
/debugfs/tracing/trace_stat which will contain all the stat files for the
current active tracer.

Now a tracer will, if desired, want to provide a zero terminated array of
tracer_stat structures.
Each one contains the callbacks necessary for one stat file.
It have to provide at least a name for its stat file, an iterator with
stat_start/start_next callback and an output callback for one stat entry.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 46677ea..b093f36 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -335,6 +335,25 @@ struct tracer_flags {
 #define TRACER_OPT(s, b)	.name = #s, .bit = b
 
 /*
+ * If you want to provide a stat file (one-shot statistics), fill
+ * an iterator with stat_start/stat_next and a stat_show callbacks.
+ * The others callbacks are optional.
+ */
+struct tracer_stat {
+	/* The name of your stat file */
+	const char		*name;
+	/* Iteration over statistic entries */
+	void			*(*stat_start)(void);
+	void			*(*stat_next)(void *prev, int idx);
+	/* Compare two entries for sorting (optional) for stats */
+	int			(*stat_cmp)(void *p1, void *p2);
+	/* Print a stat entry */
+	int			(*stat_show)(struct seq_file *s, void *p);
+	/* Print the headers of your stat entries */
+	int			(*stat_headers)(struct seq_file *s);
+};
+
+/*
  * A specific tracer, represented by methods that operate on a trace array:
  */
 struct tracer {
@@ -361,21 +380,7 @@ struct tracer {
 	struct tracer		*next;
 	int			print_max;
 	struct tracer_flags 	*flags;
-
-	/*
-	 * If you change one of the following on tracing runtime, recall
-	 * init_tracer_stat()
-	 */
-
-	/* Iteration over statistic entries */
-	void			*(*stat_start)(void);
-	void			*(*stat_next)(void *prev, int idx);
-	/* Compare two entries for sorting (optional) for stats */
-	int			(*stat_cmp)(void *p1, void *p2);
-	/* Print a stat entry */
-	int			(*stat_show)(struct seq_file *s, void *p);
-	/* Print the headers of your stat entries */
-	int			(*stat_headers)(struct seq_file *s);
+	struct tracer_stat	*stats;
 };
 
 struct trace_seq {
diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index 6f194a3..293be1c 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -21,31 +21,29 @@ struct trace_stat_list {
 	void *stat;
 };
 
-static struct trace_stat_list stat_list;
+/* A stat session is the stats output in one file */
+struct tracer_stat_session {
+	struct tracer_stat *ts;
+	struct trace_stat_list	stat_list;
+	struct mutex stat_mutex;
+};
 
-/*
- * This is a copy of the current tracer to avoid racy
- * and dangerous output while the current tracer is
- * switched.
- */
-static struct tracer current_tracer;
-
-/*
- * Protect both the current tracer and the global
- * stat list.
- */
-static DEFINE_MUTEX(stat_list_mutex);
+/* All of the sessions currently in use. Each stat file embeed one session */
+static struct tracer_stat_session **all_stat_sessions;
+static int nb_sessions;
+static struct dentry *stat_dir, **stat_files;
 
 
-static void reset_stat_list(void)
+static void reset_stat_session(struct tracer_stat_session *session)
 {
+	struct trace_stat_list *root = &session->stat_list;
 	struct trace_stat_list *node;
 	struct list_head *next;
 
-	if (list_empty(&stat_list.list))
+	if (list_empty(&root->list))
 		return;
 
-	node = list_entry(stat_list.list.next, struct trace_stat_list, list);
+	node = list_entry(root->list.next, struct trace_stat_list, list);
 	next = node->list.next;
 
 	while (&node->list != next) {
@@ -54,14 +52,65 @@ static void reset_stat_list(void)
 	}
 	kfree(node);
 
-	INIT_LIST_HEAD(&stat_list.list);
+	INIT_LIST_HEAD(&root->list);
 }
 
-void init_tracer_stat(struct tracer *trace)
+/* Called when a tracer is initialized */
+static int init_all_sessions(int nb, struct tracer_stat *ts)
+{
+	int i, j;
+	struct tracer_stat_session *session;
+
+	if (all_stat_sessions) {
+		for (i = 0; i < nb_sessions; i++) {
+			session = all_stat_sessions[i];
+			reset_stat_session(session);
+			mutex_destroy(&session->stat_mutex);
+			kfree(session);
+		}
+	}
+	all_stat_sessions = kmalloc(sizeof(struct tracer_stat_session *) * nb,
+				    GFP_KERNEL);
+	if (!all_stat_sessions)
+		return -ENOMEM;
+
+	for (i = 0; i < nb; i++) {
+		session = kmalloc(sizeof(struct tracer_stat_session) * nb,
+				  GFP_KERNEL);
+		if (!session)
+			goto free_sessions;
+
+		INIT_LIST_HEAD(&session->stat_list.list);
+		mutex_init(&session->stat_mutex);
+		session->ts = &ts[i];
+		all_stat_sessions[i] = session;
+	}
+	nb_sessions = nb;
+	return 0;
+
+free_sessions:
+
+	for (j = 0; j < i; j++)
+		kfree(all_stat_sessions[i]);
+
+	kfree(all_stat_sessions);
+	all_stat_sessions = NULL;
+
+	return -ENOMEM;
+}
+
+static int basic_tracer_stat_checks(struct tracer_stat *ts)
 {
-	mutex_lock(&stat_list_mutex);
-	current_tracer = *trace;
-	mutex_unlock(&stat_list_mutex);
+	int i;
+
+	if (!ts)
+		return 0;
+
+	for (i = 0; ts[i].name; i++) {
+		if (!ts[i].stat_start || !ts[i].stat_next || !ts[i].stat_show)
+			return -EBUSY;
+	}
+	return i;
 }
 
 /*
@@ -79,22 +128,19 @@ static int dummy_cmp(void *p1, void *p2)
  * All of these copies and sorting are required on all opening
  * since the stats could have changed between two file sessions.
  */
-static int stat_seq_init(void)
+static int stat_seq_init(struct tracer_stat_session *session)
 {
-	struct trace_stat_list *iter_entry, *new_entry;
+	struct trace_stat_list *iter_entry, *new_entry, *root;
+	struct tracer_stat *ts = session->ts;
 	void *prev_stat;
 	int ret = 0;
 	int i;
 
-	mutex_lock(&stat_list_mutex);
-	reset_stat_list();
-
-	if (!current_tracer.stat_start || !current_tracer.stat_next ||
-					!current_tracer.stat_show)
-		goto exit;
+	mutex_lock(&session->stat_mutex);
+	reset_stat_session(session);
 
-	if (!current_tracer.stat_cmp)
-		current_tracer.stat_cmp = dummy_cmp;
+	if (!ts->stat_cmp)
+		ts->stat_cmp = dummy_cmp;
 
 	/*
 	 * The first entry. Actually this is the second, but the first
@@ -107,8 +153,9 @@ static int stat_seq_init(void)
 	}
 
 	INIT_LIST_HEAD(&new_entry->list);
-	list_add(&new_entry->list, &stat_list.list);
-	new_entry->stat = current_tracer.stat_start();
+	root = &session->stat_list;
+	list_add(&new_entry->list, &root->list);
+	new_entry->stat = ts->stat_start();
 
 	prev_stat = new_entry->stat;
 
@@ -124,15 +171,15 @@ static int stat_seq_init(void)
 		}
 
 		INIT_LIST_HEAD(&new_entry->list);
-		new_entry->stat = current_tracer.stat_next(prev_stat, i);
+		new_entry->stat = ts->stat_next(prev_stat, i);
 
 		/* End of insertion */
 		if (!new_entry->stat)
 			break;
 
-		list_for_each_entry(iter_entry, &stat_list.list, list) {
+		list_for_each_entry(iter_entry, &root->list, list) {
 			/* Insertion with a descendent sorting */
-			if (current_tracer.stat_cmp(new_entry->stat,
+			if (ts->stat_cmp(new_entry->stat,
 						iter_entry->stat) > 0) {
 
 				list_add_tail(&new_entry->list,
@@ -141,7 +188,7 @@ static int stat_seq_init(void)
 
 			/* The current smaller value */
 			} else if (list_is_last(&iter_entry->list,
-						&stat_list.list)) {
+						&root->list)) {
 				list_add(&new_entry->list, &iter_entry->list);
 				break;
 			}
@@ -150,46 +197,49 @@ static int stat_seq_init(void)
 		prev_stat = new_entry->stat;
 	}
 exit:
-	mutex_unlock(&stat_list_mutex);
+	mutex_unlock(&session->stat_mutex);
 	return ret;
 
 exit_free_list:
-	reset_stat_list();
-	mutex_unlock(&stat_list_mutex);
+	reset_stat_session(session);
+	mutex_unlock(&session->stat_mutex);
 	return ret;
 }
 
 
 static void *stat_seq_start(struct seq_file *s, loff_t *pos)
 {
-	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
+	struct tracer_stat_session *session = s->private;
 
 	/* Prevent from tracer switch or stat_list modification */
-	mutex_lock(&stat_list_mutex);
+	mutex_lock(&session->stat_mutex);
 
 	/* If we are in the beginning of the file, print the headers */
-	if (!*pos && current_tracer.stat_headers)
-		current_tracer.stat_headers(s);
+	if (!*pos && session->ts->stat_headers)
+		session->ts->stat_headers(s);
 
-	return seq_list_start(&l->list, *pos);
+	return seq_list_start(&session->stat_list.list, *pos);
 }
 
 static void *stat_seq_next(struct seq_file *s, void *p, loff_t *pos)
 {
-	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
+	struct tracer_stat_session *session = s->private;
 
-	return seq_list_next(p, &l->list, pos);
+	return seq_list_next(p, &session->stat_list.list, pos);
 }
 
-static void stat_seq_stop(struct seq_file *m, void *p)
+static void stat_seq_stop(struct seq_file *s, void *p)
 {
-	mutex_unlock(&stat_list_mutex);
+	struct tracer_stat_session *session = s->private;
+	mutex_unlock(&session->stat_mutex);
 }
 
 static int stat_seq_show(struct seq_file *s, void *v)
 {
+	struct tracer_stat_session *session = s->private;
 	struct trace_stat_list *l = list_entry(v, struct trace_stat_list, list);
-	return current_tracer.stat_show(s, l->stat);
+
+	return session->ts->stat_show(s, l->stat);
 }
 
 static const struct seq_operations trace_stat_seq_ops = {
@@ -199,15 +249,18 @@ static const struct seq_operations trace_stat_seq_ops = {
 	.show = stat_seq_show
 };
 
+/* The session stat is refilled and resorted at each stat file opening */
 static int tracing_stat_open(struct inode *inode, struct file *file)
 {
 	int ret;
 
+	struct tracer_stat_session *session = inode->i_private;
+
 	ret = seq_open(file, &trace_stat_seq_ops);
 	if (!ret) {
 		struct seq_file *m = file->private_data;
-		m->private = &stat_list;
-		ret = stat_seq_init();
+		m->private = session;
+		ret = stat_seq_init(session);
 	}
 
 	return ret;
@@ -219,9 +272,11 @@ static int tracing_stat_open(struct inode *inode, struct file *file)
  */
 static int tracing_stat_release(struct inode *i, struct file *f)
 {
-	mutex_lock(&stat_list_mutex);
-	reset_stat_list();
-	mutex_unlock(&stat_list_mutex);
+	struct tracer_stat_session *session = i->i_private;
+
+	mutex_lock(&session->stat_mutex);
+	reset_stat_session(session);
+	mutex_unlock(&session->stat_mutex);
 	return 0;
 }
 
@@ -232,18 +287,70 @@ static const struct file_operations tracing_stat_fops = {
 	.release	= tracing_stat_release
 };
 
+
+static void destroy_trace_stat_files(void)
+{
+	int i;
+
+	if (stat_files) {
+		for (i = 0; i < nb_sessions; i++)
+			debugfs_remove(stat_files[i]);
+		kfree(stat_files);
+		stat_files = NULL;
+	}
+}
+
+static void init_trace_stat_files(void)
+{
+	int i;
+
+	if (!stat_dir || !nb_sessions)
+		return;
+
+	stat_files = kmalloc(sizeof(struct dentry *) * nb_sessions, GFP_KERNEL);
+
+	if (!stat_files) {
+		pr_warning("trace stat: not enough memory\n");
+		return;
+	}
+
+	for (i = 0; i < nb_sessions; i++) {
+		struct tracer_stat_session *session = all_stat_sessions[i];
+		stat_files[i] = debugfs_create_file(session->ts->name, 0644,
+						stat_dir,
+						session, &tracing_stat_fops);
+		if (!stat_files[i])
+			pr_warning("cannot create %s entry\n",
+				   session->ts->name);
+	}
+}
+
+void init_tracer_stat(struct tracer *trace)
+{
+	int nb = basic_tracer_stat_checks(trace->stats);
+
+	destroy_trace_stat_files();
+
+	if (nb < 0) {
+		pr_warning("stat tracing: missing stat callback on %s\n",
+			   trace->name);
+		return;
+	}
+	if (!nb)
+		return;
+
+	init_all_sessions(nb, trace->stats);
+	init_trace_stat_files();
+}
+
 static int __init tracing_stat_init(void)
 {
 	struct dentry *d_tracing;
-	struct dentry *entry;
 
-	INIT_LIST_HEAD(&stat_list.list);
 	d_tracing = tracing_init_dentry();
 
-	entry = debugfs_create_file("trace_stat", 0444, d_tracing,
-					NULL,
-				    &tracing_stat_fops);
-	if (!entry)
+	stat_dir = debugfs_create_dir("trace_stat", d_tracing);
+	if (!stat_dir)
 		pr_warning("Could not create debugfs "
 			   "'trace_stat' entry\n");
 	return 0;
-- 
1.6.0.4

Re: [PATCH 1/2] tracing/ftrace: handle more than one stat file per tracer

[Steven Rostedt]

On Wed, 31 Dec 2008, Frederic Weisbecker wrote:

> Impact: new API for tracers
> 
> Make the stat tracing API reentrant. And also provide the new directory
> /debugfs/tracing/trace_stat which will contain all the stat files for the
> current active tracer.
> 
> Now a tracer will, if desired, want to provide a zero terminated array of
> tracer_stat structures.
> Each one contains the callbacks necessary for one stat file.
> It have to provide at least a name for its stat file, an iterator with
> stat_start/start_next callback and an output callback for one stat entry.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 46677ea..b093f36 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -335,6 +335,25 @@ struct tracer_flags {
>  #define TRACER_OPT(s, b)	.name = #s, .bit = b
>  
>  /*
> + * If you want to provide a stat file (one-shot statistics), fill
> + * an iterator with stat_start/stat_next and a stat_show callbacks.
> + * The others callbacks are optional.
> + */
> +struct tracer_stat {
> +	/* The name of your stat file */
> +	const char		*name;
> +	/* Iteration over statistic entries */
> +	void			*(*stat_start)(void);
> +	void			*(*stat_next)(void *prev, int idx);
> +	/* Compare two entries for sorting (optional) for stats */
> +	int			(*stat_cmp)(void *p1, void *p2);
> +	/* Print a stat entry */
> +	int			(*stat_show)(struct seq_file *s, void *p);
> +	/* Print the headers of your stat entries */
> +	int			(*stat_headers)(struct seq_file *s);
> +};
> +
> +/*
>   * A specific tracer, represented by methods that operate on a trace array:
>   */
>  struct tracer {
> @@ -361,21 +380,7 @@ struct tracer {
>  	struct tracer		*next;
>  	int			print_max;
>  	struct tracer_flags 	*flags;
> -
> -	/*
> -	 * If you change one of the following on tracing runtime, recall
> -	 * init_tracer_stat()
> -	 */
> -
> -	/* Iteration over statistic entries */
> -	void			*(*stat_start)(void);
> -	void			*(*stat_next)(void *prev, int idx);
> -	/* Compare two entries for sorting (optional) for stats */
> -	int			(*stat_cmp)(void *p1, void *p2);
> -	/* Print a stat entry */
> -	int			(*stat_show)(struct seq_file *s, void *p);
> -	/* Print the headers of your stat entries */
> -	int			(*stat_headers)(struct seq_file *s);
> +	struct tracer_stat	*stats;
>  };
>  
>  struct trace_seq {
> diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
> index 6f194a3..293be1c 100644
> --- a/kernel/trace/trace_stat.c
> +++ b/kernel/trace/trace_stat.c
> @@ -21,31 +21,29 @@ struct trace_stat_list {
>  	void *stat;
>  };
>  
> -static struct trace_stat_list stat_list;
> +/* A stat session is the stats output in one file */
> +struct tracer_stat_session {
> +	struct tracer_stat *ts;
> +	struct trace_stat_list	stat_list;
> +	struct mutex stat_mutex;
> +};
>  
> -/*
> - * This is a copy of the current tracer to avoid racy
> - * and dangerous output while the current tracer is
> - * switched.
> - */
> -static struct tracer current_tracer;
> -
> -/*
> - * Protect both the current tracer and the global
> - * stat list.
> - */
> -static DEFINE_MUTEX(stat_list_mutex);
> +/* All of the sessions currently in use. Each stat file embeed one session */
> +static struct tracer_stat_session **all_stat_sessions;
> +static int nb_sessions;
> +static struct dentry *stat_dir, **stat_files;
>  
>  
> -static void reset_stat_list(void)
> +static void reset_stat_session(struct tracer_stat_session *session)
>  {
> +	struct trace_stat_list *root = &session->stat_list;
>  	struct trace_stat_list *node;
>  	struct list_head *next;
>  
> -	if (list_empty(&stat_list.list))
> +	if (list_empty(&root->list))
>  		return;
>  
> -	node = list_entry(stat_list.list.next, struct trace_stat_list, list);
> +	node = list_entry(root->list.next, struct trace_stat_list, list);
>  	next = node->list.next;
>  
>  	while (&node->list != next) {
> @@ -54,14 +52,65 @@ static void reset_stat_list(void)
>  	}
>  	kfree(node);
>  
> -	INIT_LIST_HEAD(&stat_list.list);
> +	INIT_LIST_HEAD(&root->list);
>  }
>  
> -void init_tracer_stat(struct tracer *trace)
> +/* Called when a tracer is initialized */
> +static int init_all_sessions(int nb, struct tracer_stat *ts)
> +{
> +	int i, j;
> +	struct tracer_stat_session *session;
> +
> +	if (all_stat_sessions) {
> +		for (i = 0; i < nb_sessions; i++) {
> +			session = all_stat_sessions[i];
> +			reset_stat_session(session);
> +			mutex_destroy(&session->stat_mutex);
> +			kfree(session);
> +		}
> +	}
> +	all_stat_sessions = kmalloc(sizeof(struct tracer_stat_session *) * nb,
> +				    GFP_KERNEL);

You want this to be kzalloc.

> +	if (!all_stat_sessions)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nb; i++) {
> +		session = kmalloc(sizeof(struct tracer_stat_session) * nb,
> +				  GFP_KERNEL);

Not so important, but you probably want to do it here too.

> +		if (!session)
> +			goto free_sessions;
> +
> +		INIT_LIST_HEAD(&session->stat_list.list);
> +		mutex_init(&session->stat_mutex);
> +		session->ts = &ts[i];
> +		all_stat_sessions[i] = session;
> +	}
> +	nb_sessions = nb;
> +	return 0;
> +
> +free_sessions:
> +
> +	for (j = 0; j < i; j++)
> +		kfree(all_stat_sessions[i]);

With out the kzalloc above, this may crash the kernel, because
all_stat_sessions[i] might not be NULL when they were not initialized.

> +
> +	kfree(all_stat_sessions);
> +	all_stat_sessions = NULL;
> +
> +	return -ENOMEM;
> +}
> +
> +static int basic_tracer_stat_checks(struct tracer_stat *ts)
>  {
> -	mutex_lock(&stat_list_mutex);
> -	current_tracer = *trace;
> -	mutex_unlock(&stat_list_mutex);
> +	int i;
> +
> +	if (!ts)
> +		return 0;
> +
> +	for (i = 0; ts[i].name; i++) {
> +		if (!ts[i].stat_start || !ts[i].stat_next || !ts[i].stat_show)
> +			return -EBUSY;
> +	}
> +	return i;
>  }
>  
>  /*
> @@ -79,22 +128,19 @@ static int dummy_cmp(void *p1, void *p2)
>   * All of these copies and sorting are required on all opening
>   * since the stats could have changed between two file sessions.
>   */
> -static int stat_seq_init(void)
> +static int stat_seq_init(struct tracer_stat_session *session)
>  {
> -	struct trace_stat_list *iter_entry, *new_entry;
> +	struct trace_stat_list *iter_entry, *new_entry, *root;
> +	struct tracer_stat *ts = session->ts;
>  	void *prev_stat;
>  	int ret = 0;
>  	int i;
>  
> -	mutex_lock(&stat_list_mutex);
> -	reset_stat_list();
> -
> -	if (!current_tracer.stat_start || !current_tracer.stat_next ||
> -					!current_tracer.stat_show)
> -		goto exit;
> +	mutex_lock(&session->stat_mutex);
> +	reset_stat_session(session);
>  
> -	if (!current_tracer.stat_cmp)
> -		current_tracer.stat_cmp = dummy_cmp;
> +	if (!ts->stat_cmp)
> +		ts->stat_cmp = dummy_cmp;
>  
>  	/*
>  	 * The first entry. Actually this is the second, but the first
> @@ -107,8 +153,9 @@ static int stat_seq_init(void)
>  	}
>  
>  	INIT_LIST_HEAD(&new_entry->list);
> -	list_add(&new_entry->list, &stat_list.list);
> -	new_entry->stat = current_tracer.stat_start();
> +	root = &session->stat_list;
> +	list_add(&new_entry->list, &root->list);
> +	new_entry->stat = ts->stat_start();
>  
>  	prev_stat = new_entry->stat;
>  
> @@ -124,15 +171,15 @@ static int stat_seq_init(void)
>  		}
>  
>  		INIT_LIST_HEAD(&new_entry->list);
> -		new_entry->stat = current_tracer.stat_next(prev_stat, i);
> +		new_entry->stat = ts->stat_next(prev_stat, i);
>  
>  		/* End of insertion */
>  		if (!new_entry->stat)
>  			break;
>  
> -		list_for_each_entry(iter_entry, &stat_list.list, list) {
> +		list_for_each_entry(iter_entry, &root->list, list) {
>  			/* Insertion with a descendent sorting */
> -			if (current_tracer.stat_cmp(new_entry->stat,
> +			if (ts->stat_cmp(new_entry->stat,
>  						iter_entry->stat) > 0) {
>  
>  				list_add_tail(&new_entry->list,
> @@ -141,7 +188,7 @@ static int stat_seq_init(void)
>  
>  			/* The current smaller value */
>  			} else if (list_is_last(&iter_entry->list,
> -						&stat_list.list)) {
> +						&root->list)) {
>  				list_add(&new_entry->list, &iter_entry->list);
>  				break;
>  			}
> @@ -150,46 +197,49 @@ static int stat_seq_init(void)
>  		prev_stat = new_entry->stat;
>  	}
>  exit:
> -	mutex_unlock(&stat_list_mutex);
> +	mutex_unlock(&session->stat_mutex);
>  	return ret;
>  
>  exit_free_list:
> -	reset_stat_list();
> -	mutex_unlock(&stat_list_mutex);
> +	reset_stat_session(session);
> +	mutex_unlock(&session->stat_mutex);
>  	return ret;
>  }
>  
>  
>  static void *stat_seq_start(struct seq_file *s, loff_t *pos)
>  {
> -	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
> +	struct tracer_stat_session *session = s->private;
>  
>  	/* Prevent from tracer switch or stat_list modification */
> -	mutex_lock(&stat_list_mutex);
> +	mutex_lock(&session->stat_mutex);
>  
>  	/* If we are in the beginning of the file, print the headers */
> -	if (!*pos && current_tracer.stat_headers)
> -		current_tracer.stat_headers(s);
> +	if (!*pos && session->ts->stat_headers)

Hmm, looks like you do need the kzalloc for both allocs above.

-- Steve

> +		session->ts->stat_headers(s);
>  
> -	return seq_list_start(&l->list, *pos);
> +	return seq_list_start(&session->stat_list.list, *pos);
>  }
>  
>  static void *stat_seq_next(struct seq_file *s, void *p, loff_t *pos)
>  {
> -	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
> +	struct tracer_stat_session *session = s->private;
>  
> -	return seq_list_next(p, &l->list, pos);
> +	return seq_list_next(p, &session->stat_list.list, pos);
>  }
>  
> -static void stat_seq_stop(struct seq_file *m, void *p)
> +static void stat_seq_stop(struct seq_file *s, void *p)
>  {
> -	mutex_unlock(&stat_list_mutex);
> +	struct tracer_stat_session *session = s->private;
> +	mutex_unlock(&session->stat_mutex);
>  }
>  
>  static int stat_seq_show(struct seq_file *s, void *v)
>  {
> +	struct tracer_stat_session *session = s->private;
>  	struct trace_stat_list *l = list_entry(v, struct trace_stat_list, list);
> -	return current_tracer.stat_show(s, l->stat);
> +
> +	return session->ts->stat_show(s, l->stat);
>  }
>  
>  static const struct seq_operations trace_stat_seq_ops = {
> @@ -199,15 +249,18 @@ static const struct seq_operations trace_stat_seq_ops = {
>  	.show = stat_seq_show
>  };
>  
> +/* The session stat is refilled and resorted at each stat file opening */
>  static int tracing_stat_open(struct inode *inode, struct file *file)
>  {
>  	int ret;
>  
> +	struct tracer_stat_session *session = inode->i_private;
> +
>  	ret = seq_open(file, &trace_stat_seq_ops);
>  	if (!ret) {
>  		struct seq_file *m = file->private_data;
> -		m->private = &stat_list;
> -		ret = stat_seq_init();
> +		m->private = session;
> +		ret = stat_seq_init(session);
>  	}
>  
>  	return ret;
> @@ -219,9 +272,11 @@ static int tracing_stat_open(struct inode *inode, struct file *file)
>   */
>  static int tracing_stat_release(struct inode *i, struct file *f)
>  {
> -	mutex_lock(&stat_list_mutex);
> -	reset_stat_list();
> -	mutex_unlock(&stat_list_mutex);
> +	struct tracer_stat_session *session = i->i_private;
> +
> +	mutex_lock(&session->stat_mutex);
> +	reset_stat_session(session);
> +	mutex_unlock(&session->stat_mutex);
>  	return 0;
>  }
>  
> @@ -232,18 +287,70 @@ static const struct file_operations tracing_stat_fops = {
>  	.release	= tracing_stat_release
>  };
>  
> +
> +static void destroy_trace_stat_files(void)
> +{
> +	int i;
> +
> +	if (stat_files) {
> +		for (i = 0; i < nb_sessions; i++)
> +			debugfs_remove(stat_files[i]);
> +		kfree(stat_files);
> +		stat_files = NULL;
> +	}
> +}
> +
> +static void init_trace_stat_files(void)
> +{
> +	int i;
> +
> +	if (!stat_dir || !nb_sessions)
> +		return;
> +
> +	stat_files = kmalloc(sizeof(struct dentry *) * nb_sessions, GFP_KERNEL);
> +
> +	if (!stat_files) {
> +		pr_warning("trace stat: not enough memory\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < nb_sessions; i++) {
> +		struct tracer_stat_session *session = all_stat_sessions[i];
> +		stat_files[i] = debugfs_create_file(session->ts->name, 0644,
> +						stat_dir,
> +						session, &tracing_stat_fops);
> +		if (!stat_files[i])
> +			pr_warning("cannot create %s entry\n",
> +				   session->ts->name);
> +	}
> +}
> +
> +void init_tracer_stat(struct tracer *trace)
> +{
> +	int nb = basic_tracer_stat_checks(trace->stats);
> +
> +	destroy_trace_stat_files();
> +
> +	if (nb < 0) {
> +		pr_warning("stat tracing: missing stat callback on %s\n",
> +			   trace->name);
> +		return;
> +	}
> +	if (!nb)
> +		return;
> +
> +	init_all_sessions(nb, trace->stats);
> +	init_trace_stat_files();
> +}
> +
>  static int __init tracing_stat_init(void)
>  {
>  	struct dentry *d_tracing;
> -	struct dentry *entry;
>  
> -	INIT_LIST_HEAD(&stat_list.list);
>  	d_tracing = tracing_init_dentry();
>  
> -	entry = debugfs_create_file("trace_stat", 0444, d_tracing,
> -					NULL,
> -				    &tracing_stat_fops);
> -	if (!entry)
> +	stat_dir = debugfs_create_dir("trace_stat", d_tracing);
> +	if (!stat_dir)
>  		pr_warning("Could not create debugfs "
>  			   "'trace_stat' entry\n");
>  	return 0;
> -- 
> 1.6.0.4
> 
> 
> 
> 

Re: [PATCH 1/2] tracing/ftrace: handle more than one stat file per tracer

[Frederic Weisbecker]

On Mon, Jan 05, 2009 at 03:53:38PM -0500, Steven Rostedt wrote:
> 
> On Wed, 31 Dec 2008, Frederic Weisbecker wrote:
> 
> > Impact: new API for tracers
> > 
> > Make the stat tracing API reentrant. And also provide the new directory
> > /debugfs/tracing/trace_stat which will contain all the stat files for the
> > current active tracer.
> > 
> > Now a tracer will, if desired, want to provide a zero terminated array of
> > tracer_stat structures.
> > Each one contains the callbacks necessary for one stat file.
> > It have to provide at least a name for its stat file, an iterator with
> > stat_start/start_next callback and an output callback for one stat entry.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 46677ea..b093f36 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -335,6 +335,25 @@ struct tracer_flags {
> >  #define TRACER_OPT(s, b)	.name = #s, .bit = b
> >  
> >  /*
> > + * If you want to provide a stat file (one-shot statistics), fill
> > + * an iterator with stat_start/stat_next and a stat_show callbacks.
> > + * The others callbacks are optional.
> > + */
> > +struct tracer_stat {
> > +	/* The name of your stat file */
> > +	const char		*name;
> > +	/* Iteration over statistic entries */
> > +	void			*(*stat_start)(void);
> > +	void			*(*stat_next)(void *prev, int idx);
> > +	/* Compare two entries for sorting (optional) for stats */
> > +	int			(*stat_cmp)(void *p1, void *p2);
> > +	/* Print a stat entry */
> > +	int			(*stat_show)(struct seq_file *s, void *p);
> > +	/* Print the headers of your stat entries */
> > +	int			(*stat_headers)(struct seq_file *s);
> > +};
> > +
> > +/*
> >   * A specific tracer, represented by methods that operate on a trace array:
> >   */
> >  struct tracer {
> > @@ -361,21 +380,7 @@ struct tracer {
> >  	struct tracer		*next;
> >  	int			print_max;
> >  	struct tracer_flags 	*flags;
> > -
> > -	/*
> > -	 * If you change one of the following on tracing runtime, recall
> > -	 * init_tracer_stat()
> > -	 */
> > -
> > -	/* Iteration over statistic entries */
> > -	void			*(*stat_start)(void);
> > -	void			*(*stat_next)(void *prev, int idx);
> > -	/* Compare two entries for sorting (optional) for stats */
> > -	int			(*stat_cmp)(void *p1, void *p2);
> > -	/* Print a stat entry */
> > -	int			(*stat_show)(struct seq_file *s, void *p);
> > -	/* Print the headers of your stat entries */
> > -	int			(*stat_headers)(struct seq_file *s);
> > +	struct tracer_stat	*stats;
> >  };
> >  
> >  struct trace_seq {
> > diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
> > index 6f194a3..293be1c 100644
> > --- a/kernel/trace/trace_stat.c
> > +++ b/kernel/trace/trace_stat.c
> > @@ -21,31 +21,29 @@ struct trace_stat_list {
> >  	void *stat;
> >  };
> >  
> > -static struct trace_stat_list stat_list;
> > +/* A stat session is the stats output in one file */
> > +struct tracer_stat_session {
> > +	struct tracer_stat *ts;
> > +	struct trace_stat_list	stat_list;
> > +	struct mutex stat_mutex;
> > +};
> >  
> > -/*
> > - * This is a copy of the current tracer to avoid racy
> > - * and dangerous output while the current tracer is
> > - * switched.
> > - */
> > -static struct tracer current_tracer;
> > -
> > -/*
> > - * Protect both the current tracer and the global
> > - * stat list.
> > - */
> > -static DEFINE_MUTEX(stat_list_mutex);
> > +/* All of the sessions currently in use. Each stat file embeed one session */
> > +static struct tracer_stat_session **all_stat_sessions;
> > +static int nb_sessions;
> > +static struct dentry *stat_dir, **stat_files;
> >  
> >  
> > -static void reset_stat_list(void)
> > +static void reset_stat_session(struct tracer_stat_session *session)
> >  {
> > +	struct trace_stat_list *root = &session->stat_list;
> >  	struct trace_stat_list *node;
> >  	struct list_head *next;
> >  
> > -	if (list_empty(&stat_list.list))
> > +	if (list_empty(&root->list))
> >  		return;
> >  
> > -	node = list_entry(stat_list.list.next, struct trace_stat_list, list);
> > +	node = list_entry(root->list.next, struct trace_stat_list, list);
> >  	next = node->list.next;
> >  
> >  	while (&node->list != next) {
> > @@ -54,14 +52,65 @@ static void reset_stat_list(void)
> >  	}
> >  	kfree(node);
> >  
> > -	INIT_LIST_HEAD(&stat_list.list);
> > +	INIT_LIST_HEAD(&root->list);
> >  }
> >  
> > -void init_tracer_stat(struct tracer *trace)
> > +/* Called when a tracer is initialized */
> > +static int init_all_sessions(int nb, struct tracer_stat *ts)
> > +{
> > +	int i, j;
> > +	struct tracer_stat_session *session;
> > +
> > +	if (all_stat_sessions) {
> > +		for (i = 0; i < nb_sessions; i++) {
> > +			session = all_stat_sessions[i];
> > +			reset_stat_session(session);
> > +			mutex_destroy(&session->stat_mutex);
> > +			kfree(session);
> > +		}
> > +	}
> > +	all_stat_sessions = kmalloc(sizeof(struct tracer_stat_session *) * nb,
> > +				    GFP_KERNEL);
> 
> You want this to be kzalloc.
> 
> > +	if (!all_stat_sessions)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < nb; i++) {
> > +		session = kmalloc(sizeof(struct tracer_stat_session) * nb,
> > +				  GFP_KERNEL);
> 
> Not so important, but you probably want to do it here too.
> 
> > +		if (!session)
> > +			goto free_sessions;
> > +
> > +		INIT_LIST_HEAD(&session->stat_list.list);
> > +		mutex_init(&session->stat_mutex);
> > +		session->ts = &ts[i];
> > +		all_stat_sessions[i] = session;
> > +	}
> > +	nb_sessions = nb;
> > +	return 0;
> > +
> > +free_sessions:
> > +
> > +	for (j = 0; j < i; j++)
> > +		kfree(all_stat_sessions[i]);
> 
> With out the kzalloc above, this may crash the kernel, because
> all_stat_sessions[i] might not be NULL when they were not initialized.


I'm not sure. j will not go through indexes of rooms that weren't allocated.

 
> > +
> > +	kfree(all_stat_sessions);
> > +	all_stat_sessions = NULL;
> > +
> > +	return -ENOMEM;
> > +}
> > +
> > +static int basic_tracer_stat_checks(struct tracer_stat *ts)
> >  {
> > -	mutex_lock(&stat_list_mutex);
> > -	current_tracer = *trace;
> > -	mutex_unlock(&stat_list_mutex);
> > +	int i;
> > +
> > +	if (!ts)
> > +		return 0;
> > +
> > +	for (i = 0; ts[i].name; i++) {
> > +		if (!ts[i].stat_start || !ts[i].stat_next || !ts[i].stat_show)
> > +			return -EBUSY;
> > +	}
> > +	return i;
> >  }
> >  
> >  /*
> > @@ -79,22 +128,19 @@ static int dummy_cmp(void *p1, void *p2)
> >   * All of these copies and sorting are required on all opening
> >   * since the stats could have changed between two file sessions.
> >   */
> > -static int stat_seq_init(void)
> > +static int stat_seq_init(struct tracer_stat_session *session)
> >  {
> > -	struct trace_stat_list *iter_entry, *new_entry;
> > +	struct trace_stat_list *iter_entry, *new_entry, *root;
> > +	struct tracer_stat *ts = session->ts;
> >  	void *prev_stat;
> >  	int ret = 0;
> >  	int i;
> >  
> > -	mutex_lock(&stat_list_mutex);
> > -	reset_stat_list();
> > -
> > -	if (!current_tracer.stat_start || !current_tracer.stat_next ||
> > -					!current_tracer.stat_show)
> > -		goto exit;
> > +	mutex_lock(&session->stat_mutex);
> > +	reset_stat_session(session);
> >  
> > -	if (!current_tracer.stat_cmp)
> > -		current_tracer.stat_cmp = dummy_cmp;
> > +	if (!ts->stat_cmp)
> > +		ts->stat_cmp = dummy_cmp;
> >  
> >  	/*
> >  	 * The first entry. Actually this is the second, but the first
> > @@ -107,8 +153,9 @@ static int stat_seq_init(void)
> >  	}
> >  
> >  	INIT_LIST_HEAD(&new_entry->list);
> > -	list_add(&new_entry->list, &stat_list.list);
> > -	new_entry->stat = current_tracer.stat_start();
> > +	root = &session->stat_list;
> > +	list_add(&new_entry->list, &root->list);
> > +	new_entry->stat = ts->stat_start();
> >  
> >  	prev_stat = new_entry->stat;
> >  
> > @@ -124,15 +171,15 @@ static int stat_seq_init(void)
> >  		}
> >  
> >  		INIT_LIST_HEAD(&new_entry->list);
> > -		new_entry->stat = current_tracer.stat_next(prev_stat, i);
> > +		new_entry->stat = ts->stat_next(prev_stat, i);
> >  
> >  		/* End of insertion */
> >  		if (!new_entry->stat)
> >  			break;
> >  
> > -		list_for_each_entry(iter_entry, &stat_list.list, list) {
> > +		list_for_each_entry(iter_entry, &root->list, list) {
> >  			/* Insertion with a descendent sorting */
> > -			if (current_tracer.stat_cmp(new_entry->stat,
> > +			if (ts->stat_cmp(new_entry->stat,
> >  						iter_entry->stat) > 0) {
> >  
> >  				list_add_tail(&new_entry->list,
> > @@ -141,7 +188,7 @@ static int stat_seq_init(void)
> >  
> >  			/* The current smaller value */
> >  			} else if (list_is_last(&iter_entry->list,
> > -						&stat_list.list)) {
> > +						&root->list)) {
> >  				list_add(&new_entry->list, &iter_entry->list);
> >  				break;
> >  			}
> > @@ -150,46 +197,49 @@ static int stat_seq_init(void)
> >  		prev_stat = new_entry->stat;
> >  	}
> >  exit:
> > -	mutex_unlock(&stat_list_mutex);
> > +	mutex_unlock(&session->stat_mutex);
> >  	return ret;
> >  
> >  exit_free_list:
> > -	reset_stat_list();
> > -	mutex_unlock(&stat_list_mutex);
> > +	reset_stat_session(session);
> > +	mutex_unlock(&session->stat_mutex);
> >  	return ret;
> >  }
> >  
> >  
> >  static void *stat_seq_start(struct seq_file *s, loff_t *pos)
> >  {
> > -	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
> > +	struct tracer_stat_session *session = s->private;
> >  
> >  	/* Prevent from tracer switch or stat_list modification */
> > -	mutex_lock(&stat_list_mutex);
> > +	mutex_lock(&session->stat_mutex);
> >  
> >  	/* If we are in the beginning of the file, print the headers */
> > -	if (!*pos && current_tracer.stat_headers)
> > -		current_tracer.stat_headers(s);
> > +	if (!*pos && session->ts->stat_headers)
> 
> Hmm, looks like you do need the kzalloc for both allocs above.
> 
> -- Steve


There is no check about -ENOMEM before creating the debugfs file.
So yes, I should check it before dereferencing a session, or more likely
not create the debugfs files in this case. But I don't know why kzalloc...



> 
> > +		session->ts->stat_headers(s);
> >  
> > -	return seq_list_start(&l->list, *pos);
> > +	return seq_list_start(&session->stat_list.list, *pos);
> >  }
> >  
> >  static void *stat_seq_next(struct seq_file *s, void *p, loff_t *pos)
> >  {
> > -	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
> > +	struct tracer_stat_session *session = s->private;
> >  
> > -	return seq_list_next(p, &l->list, pos);
> > +	return seq_list_next(p, &session->stat_list.list, pos);
> >  }
> >  
> > -static void stat_seq_stop(struct seq_file *m, void *p)
> > +static void stat_seq_stop(struct seq_file *s, void *p)
> >  {
> > -	mutex_unlock(&stat_list_mutex);
> > +	struct tracer_stat_session *session = s->private;
> > +	mutex_unlock(&session->stat_mutex);
> >  }
> >  
> >  static int stat_seq_show(struct seq_file *s, void *v)
> >  {
> > +	struct tracer_stat_session *session = s->private;
> >  	struct trace_stat_list *l = list_entry(v, struct trace_stat_list, list);
> > -	return current_tracer.stat_show(s, l->stat);
> > +
> > +	return session->ts->stat_show(s, l->stat);
> >  }
> >  
> >  static const struct seq_operations trace_stat_seq_ops = {
> > @@ -199,15 +249,18 @@ static const struct seq_operations trace_stat_seq_ops = {
> >  	.show = stat_seq_show
> >  };
> >  
> > +/* The session stat is refilled and resorted at each stat file opening */
> >  static int tracing_stat_open(struct inode *inode, struct file *file)
> >  {
> >  	int ret;
> >  
> > +	struct tracer_stat_session *session = inode->i_private;
> > +
> >  	ret = seq_open(file, &trace_stat_seq_ops);
> >  	if (!ret) {
> >  		struct seq_file *m = file->private_data;
> > -		m->private = &stat_list;
> > -		ret = stat_seq_init();
> > +		m->private = session;
> > +		ret = stat_seq_init(session);
> >  	}
> >  
> >  	return ret;
> > @@ -219,9 +272,11 @@ static int tracing_stat_open(struct inode *inode, struct file *file)
> >   */
> >  static int tracing_stat_release(struct inode *i, struct file *f)
> >  {
> > -	mutex_lock(&stat_list_mutex);
> > -	reset_stat_list();
> > -	mutex_unlock(&stat_list_mutex);
> > +	struct tracer_stat_session *session = i->i_private;
> > +
> > +	mutex_lock(&session->stat_mutex);
> > +	reset_stat_session(session);
> > +	mutex_unlock(&session->stat_mutex);
> >  	return 0;
> >  }
> >  
> > @@ -232,18 +287,70 @@ static const struct file_operations tracing_stat_fops = {
> >  	.release	= tracing_stat_release
> >  };
> >  
> > +
> > +static void destroy_trace_stat_files(void)
> > +{
> > +	int i;
> > +
> > +	if (stat_files) {
> > +		for (i = 0; i < nb_sessions; i++)
> > +			debugfs_remove(stat_files[i]);
> > +		kfree(stat_files);
> > +		stat_files = NULL;
> > +	}
> > +}
> > +
> > +static void init_trace_stat_files(void)
> > +{
> > +	int i;
> > +
> > +	if (!stat_dir || !nb_sessions)
> > +		return;
> > +
> > +	stat_files = kmalloc(sizeof(struct dentry *) * nb_sessions, GFP_KERNEL);
> > +
> > +	if (!stat_files) {
> > +		pr_warning("trace stat: not enough memory\n");
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i < nb_sessions; i++) {
> > +		struct tracer_stat_session *session = all_stat_sessions[i];
> > +		stat_files[i] = debugfs_create_file(session->ts->name, 0644,
> > +						stat_dir,
> > +						session, &tracing_stat_fops);
> > +		if (!stat_files[i])
> > +			pr_warning("cannot create %s entry\n",
> > +				   session->ts->name);
> > +	}
> > +}
> > +
> > +void init_tracer_stat(struct tracer *trace)
> > +{
> > +	int nb = basic_tracer_stat_checks(trace->stats);
> > +
> > +	destroy_trace_stat_files();
> > +
> > +	if (nb < 0) {
> > +		pr_warning("stat tracing: missing stat callback on %s\n",
> > +			   trace->name);
> > +		return;
> > +	}
> > +	if (!nb)
> > +		return;
> > +
> > +	init_all_sessions(nb, trace->stats);
> > +	init_trace_stat_files();
> > +}
> > +
> >  static int __init tracing_stat_init(void)
> >  {
> >  	struct dentry *d_tracing;
> > -	struct dentry *entry;
> >  
> > -	INIT_LIST_HEAD(&stat_list.list);
> >  	d_tracing = tracing_init_dentry();
> >  
> > -	entry = debugfs_create_file("trace_stat", 0444, d_tracing,
> > -					NULL,
> > -				    &tracing_stat_fops);
> > -	if (!entry)
> > +	stat_dir = debugfs_create_dir("trace_stat", d_tracing);
> > +	if (!stat_dir)
> >  		pr_warning("Could not create debugfs "
> >  			   "'trace_stat' entry\n");
> >  	return 0;
> > -- 
> > 1.6.0.4
> > 
> > 
> > 
> > 

Re: [PATCH 1/2] tracing/ftrace: handle more than one stat file per tracer

[Steven Rostedt]

On Tue, 6 Jan 2009, Frederic Weisbecker wrote:
> > > +
> > > +	if (all_stat_sessions) {
> > > +		for (i = 0; i < nb_sessions; i++) {
> > > +			session = all_stat_sessions[i];
> > > +			reset_stat_session(session);
> > > +			mutex_destroy(&session->stat_mutex);
> > > +			kfree(session);
> > > +		}
> > > +	}
> > > +	all_stat_sessions = kmalloc(sizeof(struct tracer_stat_session *) * nb,
> > > +				    GFP_KERNEL);
> > 
> > You want this to be kzalloc.
> > 
> > > +	if (!all_stat_sessions)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < nb; i++) {
> > > +		session = kmalloc(sizeof(struct tracer_stat_session) * nb,
> > > +				  GFP_KERNEL);
> > 
> > Not so important, but you probably want to do it here too.
> > 
> > > +		if (!session)
> > > +			goto free_sessions;
> > > +
> > > +		INIT_LIST_HEAD(&session->stat_list.list);
> > > +		mutex_init(&session->stat_mutex);
> > > +		session->ts = &ts[i];
> > > +		all_stat_sessions[i] = session;
> > > +	}
> > > +	nb_sessions = nb;
> > > +	return 0;
> > > +
> > > +free_sessions:
> > > +
> > > +	for (j = 0; j < i; j++)
> > > +		kfree(all_stat_sessions[i]);
> > 
> > With out the kzalloc above, this may crash the kernel, because
> > all_stat_sessions[i] might not be NULL when they were not initialized.
> 
> 
> I'm not sure. j will not go through indexes of rooms that weren't allocated.

Ah, OK, my eyes confused i's and j's so I did not notice the loop to last 
failed. Which is strange because I write similar code :-/.

-- Steve