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
