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

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Nov 19, 2015 at 11:17:26PM -0500, Steven Rostedt wrote:
> 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?

Ummm...  Pretty crazy.  ;-)

For me, it was mostly confusing, as I could not figure out how the
rcu_read_lock_sched() was helping.  But of course, others' mileage
might vary.

							Thanx, Paul

> -- 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;
> > > +}  
>