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: Wed, 20 May 2020 11:43:45 -0700
Message-ID: <20200520184345.GU2869@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200520120608.mwros5jurmidxxfv@linutronix.de>

On Wed, May 20, 2020 at 02:06:08PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-20 12:24:07 [+0200], Peter Zijlstra wrote:
> > On Tue, May 19, 2020 at 10:19:07PM +0200, Sebastian Andrzej Siewior wrote:
> > 
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 0c71505f0e19c..8d2b5f75145d7 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/module.h>
> > >  #include <linux/srcu.h>
> > > +#include <linux/locallock.h>
> > >  
> > >  #include "rcu.h"
> > >  #include "rcu_segcblist.h"
> > > @@ -735,6 +736,7 @@ static void srcu_flip(struct srcu_struct *ssp)
> > >  	smp_mb(); /* D */  /* Pairs with C. */
> > >  }
> > >  
> > > +static DEFINE_LOCAL_LOCK(sda_lock);
> > >  /*
> > >   * If SRCU is likely idle, return true, otherwise return false.
> > >   *
> > > @@ -765,13 +767,13 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
> > >  	unsigned long tlast;
> > >  
> > >  	/* If the local srcu_data structure has callbacks, not idle.  */
> > > -	local_irq_save(flags);
> > > +	local_lock_irqsave(sda_lock, flags);
> > >  	sdp = this_cpu_ptr(ssp->sda);
> > >  	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
> > > -		local_irq_restore(flags);
> > > +		local_unlock_irqrestore(sda_lock, flags);
> > >  		return false; /* Callbacks already present, so not idle. */
> > >  	}
> > > -	local_irq_restore(flags);
> > > +	local_unlock_irqrestore(sda_lock, flags);
> > 
> > Would it perhaps make sense to stick the local_lock in struct srcu_data ?
> 
> In that case we would need something for pointer stability before the
> lock is acquired.
> I remember Paul looked at that patch a few years ago and he said that
> that disabling interrupts here is important and matches the other part
> instance where the interrupts are disabled. Looking at it now, it seems
> that there is just pointer stability but I can't tell if
> rcu_segcblist_pend_cbs() needs more than just this.

Yes, that CPU's rcu_segcblist structure does need mutual exclusion in
this case.  This is because rcu_segcblist_pend_cbs() looks not just
at the ->tails[] pointer, but also at the pointer referenced by the
->tails[] pointer.  This last pointer is in an rcu_head structure, and
not just any rcu_head structure, but one that is ready to be invoked.
So this callback could vanish into the freelist (or worse) at any time.
But callback invocation runs on the CPU that enqueued the callbacks
(as long as that CPU remains online, anyway), so disabling interrupts
suffices in mainline.

Now, we could have srcu_might_be_idle() instead acquire the sdp->lock
to protect the structure.

What would be really nice is a primitive that acquires such a per-CPU
lock and remains executing on that CPU, whether by the graces of
preempt_disable(), local_irq_save(), migrate_disable(), or what have you.

							Thanx, Paul

  parent 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 [this message]
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
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=20200520184345.GU2869@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