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" <paul...@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; > > > +} > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/