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? -- 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/