RCU Archive on lore.kernel.org
 help / color / Atom feed
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
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

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200519201912.1564477-1-bigeasy@linutronix.de>
2020-05-19 20:19 ` Sebastian Andrzej Siewior
2020-05-20 10:24   ` Peter Zijlstra
2020-05-20 12:06     ` Sebastian Andrzej Siewior
2020-05-20 13:27       ` Peter Zijlstra
2020-05-20 17:42       ` Joel Fernandes
2020-05-20 18:28         ` Sebastian Andrzej Siewior
2020-05-20 18:35           ` Peter Zijlstra
2020-05-20 18:44             ` Joel Fernandes
2020-05-20 18:50               ` Uladzislau Rezki
2020-05-20 18:59           ` Joel Fernandes
2020-05-20 18:43       ` Paul E. McKenney
2020-05-22 15:12         ` Sebastian Andrzej Siewior
2020-05-22 17:39           ` Paul E. McKenney
2020-05-23 15:08             ` Sebastian Andrzej Siewior
2020-05-23 16:59               ` Paul E. McKenney [this message]
2020-05-24 19:03               ` Sebastian Andrzej Siewior
2020-05-25  3:27                 ` Paul E. McKenney
2020-05-26 13:41                   ` [PATCH] srcu: Avoid local_irq_save() before acquiring spinlock_t Sebastian Andrzej Siewior
2020-05-26 16:16                     ` Paul E. McKenney
2020-05-26 16:31                       ` Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200523165911.GQ2869@paulmck-ThinkPad-P72 \
    --to=paulmck@kernel.org \
    --cc=bigeasy@linutronix.de \
    --cc=jiangshanlai@gmail.com \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rcu@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

RCU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/rcu/0 rcu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 rcu rcu/ https://lore.kernel.org/rcu \
		rcu@vger.kernel.org
	public-inbox-index rcu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.rcu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git