lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 23 May 2020 09:59:11 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Will Deacon <will@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Josh Triplett <josh@...htriplett.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        rcu@...r.kernel.org
Subject: Re: [PATCH 3/8] srcu: Use local_lock() for per-CPU struct srcu_data
 access

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

Powered by blists - more mailing lists