Re: [PATCH 3/8] srcu: Use local_lock() for per-CPU struct srcu_data access

From: "Paul E. McKenney" <paulmck@kernel.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Will Deacon <will@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Josh Triplett <josh@joshtriplett.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	rcu@vger.kernel.org
Subject: Re: [PATCH 3/8] srcu: Use local_lock() for per-CPU struct srcu_data access
Date: Sat, 23 May 2020 09:59:11 -0700	[thread overview]
Message-ID: <20200523165911.GQ2869@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200523150831.wdrthklakwm6wago@linutronix.de>

On Sat, May 23, 2020 at 05:08:31PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-22 10:39:53 [-0700], Paul E. McKenney wrote:
> > It looks good to me, but I have not yet tested it.  (Happy to let you
> > take the first crack at rcutorture in any case, scenarios SRCU-P and
> > SRCU-N.)
> 
> on it.
> 
> > > That check_init_srcu_struct() is needed, because otherwise:
> > > 
> > > | BUG: spinlock bad magic on CPU#2, swapper/0/1
> > > |  lock: 0xffff88803ed28ac0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
> > > | CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc6+ #81
> > > | Call Trace:
> > > |  dump_stack+0x71/0xa0
> > > |  do_raw_spin_lock+0x6c/0xb0
> > > |  _raw_spin_lock_irqsave+0x33/0x40
> > > |  synchronize_srcu+0x24/0xc9
> > > |  wakeup_source_remove+0x4d/0x70
> > > |  wakeup_source_unregister.part.0+0x9/0x40
> > > |  device_wakeup_enable+0x99/0xc0
> > > 
> > > I'm not sure if there should be an explicit init of `wakeup_srcu' or if
> > > an srcu function (like call_srcu()) is supposed to do it.
> > 
> > It is fine.  Beforehand, that check_init_srcu_struct() would have been
> > invoked very shortly thereafter from __call_srcu(), and there is no
> > instead harm invoking it a few microseconds earlier.  ;-)
> 
> Oki. I wasn't sure if an explizit initialized on wakeup_srcu's side was
> missing or if this is new since we use the lock earlier.

If you want to worry, the cause for concern that comes to mind is lock
contention.  Except that this is a per-CPU lock, and it isn't acquired
all that often.  And when it is acquired often (call_srcu() floods, for
example), it is acquired on the CPU in question.  So seems unlikely.

But should lock contention nevertheless become a problem:

1.	Tighten up any acquisitions to avoid unncessary off-CPU
	acquisition of this lock.

2.	Convert this particular lock acquisition into a trylock,
	and make trylock failure return such that a non-expedited
	grace period results.

But again, I don't see the need for these at the moment.

							Thanx, Paul