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

[Steven Rostedt]

On Thu, 19 Nov 2015 15:24:27 -0800
"Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/