Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1273587 > unrolled thread

Re: [for-next][PATCH 2/5] tracing: Add set_event_pid directory for future use

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [for-next][PATCH 2/5] tracing: Add set_event_pid directory for  future use "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-20 00:30 +0100
    Re: [for-next][PATCH 2/5] tracing: Add set_event_pid directory for  future use Steven Rostedt <rostedt@goodmis.org> - 2015-11-20 05:20 +0100

#1273587 — Re: [for-next][PATCH 2/5] tracing: Add set_event_pid directory for future use

On Thu, Oct 29, 2015 at 03:07:56AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Create a tracing directory called set_event_pid, which currently has no
> function, but will be used to filter all events for the tracing instance or
> the pids that are added to the file.
> 
> The reason no functionality is added with this commit is that this commit
> focuses on the creation and removal of the pids in a safe manner. And tests
> can be made against this change to make sure things are correct before
> hooking features to the list of pids.
> 
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.h        |   7 ++
>  kernel/trace/trace_events.c | 287 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 294 insertions(+)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index fb8a61c710ea..250481043bb5 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -176,6 +176,12 @@ struct trace_options {
>  	struct trace_option_dentry	*topts;
>  };
> 
> +struct trace_pid_list {
> +	unsigned int			nr_pids;
> +	int				order;
> +	pid_t				*pids;
> +};
> +
>  /*
>   * The trace array - an array of per-CPU trace arrays. This is the
>   * highest level data structure that individual tracers deal with.
> @@ -201,6 +207,7 @@ struct trace_array {
>  	bool			allocated_snapshot;
>  	unsigned long		max_latency;
>  #endif
> +	struct trace_pid_list	__rcu *filtered_pids;
>  	/*
>  	 * max_lock is used to protect the swapping of buffers
>  	 * when taking a max snapshot. The buffers themselves are
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index d120cfe3cca7..2ad7014707ee 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -15,8 +15,10 @@
>  #include <linux/kthread.h>
>  #include <linux/tracefs.h>
>  #include <linux/uaccess.h>
> +#include <linux/bsearch.h>
>  #include <linux/module.h>
>  #include <linux/ctype.h>
> +#include <linux/sort.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> 
> @@ -445,6 +447,43 @@ static void ftrace_clear_events(struct trace_array *tr)
>  	mutex_unlock(&event_mutex);
>  }
> 
> +static int cmp_pid(const void *key, const void *elt)
> +{
> +	const pid_t *search_pid = key;
> +	const pid_t *pid = elt;
> +
> +	if (*search_pid == *pid)
> +		return 0;
> +	if (*search_pid < *pid)
> +		return -1;
> +	return 1;
> +}
> +
> +static void __ftrace_clear_event_pids(struct trace_array *tr)
> +{
> +	struct trace_pid_list *pid_list;
> +
> +	pid_list = rcu_dereference_protected(tr->filtered_pids,
> +					     lockdep_is_held(&event_mutex));
> +	if (!pid_list)
> +		return;
> +
> +	rcu_assign_pointer(tr->filtered_pids, NULL);
> +
> +	/* Wait till all users are no longer using pid filtering */
> +	synchronize_sched();
> +
> +	free_pages((unsigned long)pid_list->pids, pid_list->order);
> +	kfree(pid_list);
> +}
> +
> +static void ftrace_clear_event_pids(struct trace_array *tr)
> +{
> +	mutex_lock(&event_mutex);
> +	__ftrace_clear_event_pids(tr);
> +	mutex_unlock(&event_mutex);
> +}
> +
>  static void __put_system(struct event_subsystem *system)
>  {
>  	struct event_filter *filter = system->filter;
> @@ -777,6 +816,56 @@ static void t_stop(struct seq_file *m, void *p)
>  	mutex_unlock(&event_mutex);
>  }
> 
> +static void *p_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct trace_pid_list *pid_list;
> +	struct trace_array *tr = m->private;
> +
> +	/*
> +	 * Grab the mutex, to keep calls to p_next() having the same
> +	 * tr->filtered_pids as p_start() has.
> +	 * If we just passed the tr->filtered_pids around, then RCU would
> +	 * have been enough, but doing that makes things more complex.
> +	 */
> +	mutex_lock(&event_mutex);
> +	rcu_read_lock_sched();

This looks interesting...  You hold the mutex, which I am guessing
blocks changes.  Then why the need for rcu_read_lock_sched()?

						Thanx, Paul

> +
> +	pid_list = rcu_dereference_sched(tr->filtered_pids);
> +
> +	if (!pid_list || *pos >= pid_list->nr_pids)
> +		return NULL;
> +
> +	return (void *)&pid_list->pids[*pos];
> +}
> +
> +static void p_stop(struct seq_file *m, void *p)
> +{
> +	rcu_read_unlock_sched();
> +	mutex_unlock(&event_mutex);
> +}
> +
> +static void *
> +p_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	struct trace_array *tr = m->private;
> +	struct trace_pid_list *pid_list = rcu_dereference_sched(tr->filtered_pids);
> +
> +	(*pos)++;
> +
> +	if (*pos >= pid_list->nr_pids)
> +		return NULL;
> +
> +	return (void *)&pid_list->pids[*pos];
> +}
> +
> +static int p_show(struct seq_file *m, void *v)
> +{
> +	pid_t *pid = v;
> +
> +	seq_printf(m, "%d\n", *pid);
> +	return 0;
> +}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1273718

On Thu, 19 Nov 2015 15:24:27 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > +static void *p_start(struct seq_file *m, loff_t *pos)
> > +{
> > +	struct trace_pid_list *pid_list;
> > +	struct trace_array *tr = m->private;
> > +
> > +	/*
> > +	 * Grab the mutex, to keep calls to p_next() having the same
> > +	 * tr->filtered_pids as p_start() has.
> > +	 * If we just passed the tr->filtered_pids around, then RCU would
> > +	 * have been enough, but doing that makes things more complex.
> > +	 */
> > +	mutex_lock(&event_mutex);
> > +	rcu_read_lock_sched();  
> 
> This looks interesting...  You hold the mutex, which I am guessing
> blocks changes.  Then why the need for rcu_read_lock_sched()?

Technically you are correct. It's not needed. But I added it more for
documentation :-)

Ideally, we wouldn't need the mutex here. But then we need to pass
around the pid_list which makes it a bit more complex in the seq_file
code than to pass around the tr (where we get pid_list from
tr->filtered_pids).

And we do multiple rcu_dereference_sched()s, and for this code to work
properly (give consistent output), the result should be the same.
Hence, we grab the mutex, to keep the tr->filtered_pids to be
consistent between the rcu_dereference_sched() calls, but since we are
not modifying tr->filtered_pids(), and if we changed this code to do a
single rcu_dereference_sched() and pass around the result, then we
wouldn't need to grab the mutex, and the rcu_read_lock_sched() would be
enough.

I could remove it and change the code to do rcu_dereferenced_lock() but
to me that makes it sound like this code is an update path, which it is
not.

Does this make sense in a crazy way?

-- Steve


> 
> 						Thanx, Paul
> 
> > +
> > +	pid_list = rcu_dereference_sched(tr->filtered_pids);
> > +
> > +	if (!pid_list || *pos >= pid_list->nr_pids)
> > +		return NULL;
> > +
> > +	return (void *)&pid_list->pids[*pos];
> > +}
> > +
> > +static void p_stop(struct seq_file *m, void *p)
> > +{
> > +	rcu_read_unlock_sched();
> > +	mutex_unlock(&event_mutex);
> > +}
> > +
> > +static void *
> > +p_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > +	struct trace_array *tr = m->private;
> > +	struct trace_pid_list *pid_list = rcu_dereference_sched(tr->filtered_pids);
> > +
> > +	(*pos)++;
> > +
> > +	if (*pos >= pid_list->nr_pids)
> > +		return NULL;
> > +
> > +	return (void *)&pid_list->pids[*pos];
> > +}
> > +
> > +static int p_show(struct seq_file *m, void *v)
> > +{
> > +	pid_t *pid = v;
> > +
> > +	seq_printf(m, "%d\n", *pid);
> > +	return 0;
> > +}  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web