From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161996AbbKTESE (ORCPT ); Thu, 19 Nov 2015 23:18:04 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.232]:47406 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1161978AbbKTERb (ORCPT ); Thu, 19 Nov 2015 23:17:31 -0500 Date: Thu, 19 Nov 2015 23:17:26 -0500 From: Steven Rostedt To: "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Andrew Morton Subject: Re: [for-next][PATCH 2/5] tracing: Add set_event_pid directory for future use Message-ID: <20151119231726.5362295f@grimm.local.home> In-Reply-To: <20151119232427.GZ5184@linux.vnet.ibm.com> References: <20151029070754.584668951@goodmis.org> <20151029071032.010005111@goodmis.org> <20151119232427.GZ5184@linux.vnet.ibm.com> X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.28; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-RR-Connecting-IP: 107.14.168.118:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 19 Nov 2015 15:24:27 -0800 "Paul E. McKenney" 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; > > +}