From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756749AbbLASMD (ORCPT ); Tue, 1 Dec 2015 13:12:03 -0500 Received: from e37.co.us.ibm.com ([32.97.110.158]:43234 "EHLO e37.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753901AbbLASMA (ORCPT ); Tue, 1 Dec 2015 13:12:00 -0500 X-IBM-Helo: d03dlp02.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Tue, 1 Dec 2015 10:12:37 -0800 From: "Paul E. McKenney" To: Steven Rostedt 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: <20151201181237.GQ26643@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20151029070754.584668951@goodmis.org> <20151029071032.010005111@goodmis.org> <20151119232427.GZ5184@linux.vnet.ibm.com> <20151119231726.5362295f@grimm.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151119231726.5362295f@grimm.local.home> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15120118-0025-0000-0000-00001F3507DE Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" 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; > > > +} >