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

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

[Sebastian Andrzej Siewior]

SRCU disables interrupts to get a stable per-CPU pointer and then
acquires the spinlock which is in the per-CPU data structure. The
release uses spin_unlock_irqrestore(). While this is correct on a non-RT
kernel, this conflicts with the RT semantics because the spinlock is
converted to a 'sleeping' spinlock. Sleeping locks can obviously not be
acquired with interrupts disabled.

Add a local lock and use the corresponding local lock operations.  Split
the restore into unlock and local_lock_irqrestore(). The local lock
operations map to local_irq_disable/enable() on a non-RT kernel. On a RT
kernel the local lock is substituted with a real per CPU lock which
serializes the access and guarantees CPU locality, but keeps the code
section preemptible. No functional change.

Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: rcu@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/rcu/srcutree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

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);
 
 	/*
 	 * No local callbacks, so probabalistically probe global state.
@@ -851,7 +853,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	}
 	rhp->func = func;
 	idx = srcu_read_lock(ssp);
-	local_irq_save(flags);
+	local_lock_irqsave(sda_lock, flags);
 	sdp = this_cpu_ptr(ssp->sda);
 	spin_lock_rcu_node(sdp);
 	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
@@ -867,7 +869,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 		sdp->srcu_gp_seq_needed_exp = s;
 		needexp = true;
 	}
-	spin_unlock_irqrestore_rcu_node(sdp, flags);
+	spin_unlock_rcu_node(sdp);
+	local_unlock_irqrestore(sda_lock, flags);
 	if (needgp)
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)
-- 
2.26.2

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

[Peter Zijlstra]

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 ?

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

[Sebastian Andrzej Siewior]

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.

Sebastian

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

[Peter Zijlstra]

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:

> > 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.

Maybe I need sleep; but I think this will work.

 &x->foo = x + foo-offset

 this_cpu_ptr(x) = x + cpu-offset

 &this_cpu_ptr(x)->foo = (x + cpu-offset) + foo-offset
                       = (x + foo-offset) + cpu-offset
		       = this_cpu_ptr(&x->foo)

---
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -13,6 +13,7 @@
 
 #include <linux/rcu_node_tree.h>
 #include <linux/completion.h>
+#include <linux/locallock.h>
 
 struct srcu_node;
 struct srcu_struct;
@@ -22,6 +23,8 @@ struct srcu_struct;
  * to rcu_node.
  */
 struct srcu_data {
+	struct local_lock llock;
+
 	/* Read-side state. */
 	unsigned long srcu_lock_count[2];	/* Locks per CPU. */
 	unsigned long srcu_unlock_count[2];	/* Unlocks per CPU. */
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -778,13 +778,13 @@ static bool srcu_might_be_idle(struct sr
 	unsigned long tlast;
 
 	/* If the local srcu_data structure has callbacks, not idle.  */
-	local_irq_save(flags);
+	local_lock_irqsave(&ssp->sda->llock, flags);
 	sdp = this_cpu_ptr(ssp->sda);
 	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
-		local_irq_restore(flags);
+		local_unlock_irqrestore(&ssp->sda->llock, flags);
 		return false; /* Callbacks already present, so not idle. */
 	}
-	local_irq_restore(flags);
+	local_unlock_irqrestore(&ssp->sda->llock, flags);
 
 	/*
 	 * No local callbacks, so probabalistically probe global state.
@@ -864,7 +864,7 @@ static void __call_srcu(struct srcu_stru
 	}
 	rhp->func = func;
 	idx = srcu_read_lock(ssp);
-	local_irq_save(flags);
+	local_lock_irqsave(&ssp->sda->llock, flags);
 	sdp = this_cpu_ptr(ssp->sda);
 	spin_lock_rcu_node(sdp);
 	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
@@ -880,7 +880,8 @@ static void __call_srcu(struct srcu_stru
 		sdp->srcu_gp_seq_needed_exp = s;
 		needexp = true;
 	}
-	spin_unlock_irqrestore_rcu_node(sdp, flags);
+	spin_unlock_rcu_node(sdp);
+	local_unlock_irqrestore(&ssp->sda->llock, flags);
 	if (needgp)
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)

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

[Joel Fernandes]

Hi Sebastian,

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.

For pointer stability, can we just use get_local_ptr() and put_local_ptr()
instead of adding an extra lock? This keeps the pointer stable while keeping
the section preemptible on -rt. And we already have a lock in rcu_data, I
prefer not to add another lock if possible.

I wrote a diff below with get_local_ptr() (just build tested). Does this
solve your issue?

> 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.

Which 'other part' are you referring to? Your patch removed local_irq_save()
from other places as well right?

thanks,

 - Joel

---8<-----------------------

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 8ff71e5d0fe8b..5f49919205317 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -778,13 +778,17 @@ 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);
-	sdp = this_cpu_ptr(ssp->sda);
+	sdp = get_local_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
+
 	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
-		local_irq_restore(flags);
+		spin_unlock_irqrestore_rcu_node(sdp, flags);
+		put_local_ptr(sdp);
 		return false; /* Callbacks already present, so not idle. */
 	}
-	local_irq_restore(flags);
+
+	spin_unlock_irqrestore_rcu_node(sdp, flags);
+	put_local_ptr(sdp);
 
 	/*
 	 * No local callbacks, so probabalistically probe global state.
@@ -864,9 +868,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	}
 	rhp->func = func;
 	idx = srcu_read_lock(ssp);
-	local_irq_save(flags);
-	sdp = this_cpu_ptr(ssp->sda);
-	spin_lock_rcu_node(sdp);
+	sdp = get_local_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
 	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_gp_seq));
@@ -886,6 +889,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp->mynode, s);
 	srcu_read_unlock(ssp, idx);
+
+	put_local_ptr(sdp);
 }
 
 /**

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

[Sebastian Andrzej Siewior]

On 2020-05-20 13:42:59 [-0400], Joel Fernandes wrote:
> Hi Sebastian,
Hi Joel,

> For pointer stability, can we just use get_local_ptr() and put_local_ptr()
> instead of adding an extra lock? This keeps the pointer stable while keeping
> the section preemptible on -rt. And we already have a lock in rcu_data, I
> prefer not to add another lock if possible.

What is this get_local_ptr() doing? I can't find it anywhere…

> I wrote a diff below with get_local_ptr() (just build tested). Does this
> solve your issue?

see below.

> > 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.
> 
> Which 'other part' are you referring to? Your patch removed local_irq_save()
> from other places as well right?

The patch converted hunks.

> thanks,
> 
>  - Joel
> 
> ---8<-----------------------
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 8ff71e5d0fe8b..5f49919205317 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -778,13 +778,17 @@ 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);
> -	sdp = this_cpu_ptr(ssp->sda);
> +	sdp = get_local_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);

You acquire the node lock which was not acquired before. Is that okay?
How is get_local_ptr() different to raw_cpu_ptr()?

>  	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
> -		local_irq_restore(flags);
> +		spin_unlock_irqrestore_rcu_node(sdp, flags);
> +		put_local_ptr(sdp);
>  		return false; /* Callbacks already present, so not idle. */
>  	}
> -	local_irq_restore(flags);
> +
> +	spin_unlock_irqrestore_rcu_node(sdp, flags);
> +	put_local_ptr(sdp);
>  
>  	/*
>  	 * No local callbacks, so probabalistically probe global state.

Sebastian

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

[Peter Zijlstra]

On Wed, May 20, 2020 at 08:28:00PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-20 13:42:59 [-0400], Joel Fernandes wrote:
> > Hi Sebastian,
> Hi Joel,
> 
> > For pointer stability, can we just use get_local_ptr() and put_local_ptr()
> > instead of adding an extra lock? This keeps the pointer stable while keeping
> > the section preemptible on -rt. And we already have a lock in rcu_data, I
> > prefer not to add another lock if possible.
> 
> What is this get_local_ptr() doing? I can't find it anywhere…

I suspect it is ({ preempt_disable(); this_cpu_ptr(ptr); }), or
something along those lines.

But yeah, I can't find it either.

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

[Joel Fernandes]

On Wed, May 20, 2020 at 08:35:29PM +0200, Peter Zijlstra wrote:
> On Wed, May 20, 2020 at 08:28:00PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2020-05-20 13:42:59 [-0400], Joel Fernandes wrote:
> > > Hi Sebastian,
> > Hi Joel,
> > 
> > > For pointer stability, can we just use get_local_ptr() and put_local_ptr()
> > > instead of adding an extra lock? This keeps the pointer stable while keeping
> > > the section preemptible on -rt. And we already have a lock in rcu_data, I
> > > prefer not to add another lock if possible.
> > 
> > What is this get_local_ptr() doing? I can't find it anywhere…
> 
> I suspect it is ({ preempt_disable(); this_cpu_ptr(ptr); }), or
> something along those lines.
> 
> But yeah, I can't find it either.

I actually found it in RT 4.4 kernel, I thought this was also on newer RT
kernels as well (is that not true anymore?). But yes it was exactly what
Peter said.

In 4.4 RT there's code similar to:

#ifdef CONFIG_PREEMPT_RT_FULL

# define get_local_ptr(var) ({          \
                migrate_disable();      \
                this_cpu_ptr(var);      })

# define put_local_ptr(var) do {        \
        (void)(var);                    \
        migrate_enable();               \
} while (0)

#else

#define get_local_ptr(var)      get_cpu_ptr(var)
#define put_local_ptr(var)      put_cpu_ptr(var)

#endif


thanks,

 - Joel

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

[Uladzislau Rezki]

>
> I actually found it in RT 4.4 kernel, I thought this was also on newer RT
> kernels as well (is that not true anymore?). But yes it was exactly what
> Peter said.
> 
I see it also in 5.6.4 linut-rt-devel:

<snip>
#ifdef CONFIG_PREEMPT_RT
...
# define get_local_ptr(var) ({ \
 migrate_disable(); \
 this_cpu_ptr(var); })
...
<snip>

--
Vlad Rezki

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

[Joel Fernandes]

On Wed, May 20, 2020 at 08:28:00PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-20 13:42:59 [-0400], Joel Fernandes wrote:
> > Hi Sebastian,
> Hi Joel,
> 
> > For pointer stability, can we just use get_local_ptr() and put_local_ptr()
> > instead of adding an extra lock? This keeps the pointer stable while keeping
> > the section preemptible on -rt. And we already have a lock in rcu_data, I
> > prefer not to add another lock if possible.
> 
> What is this get_local_ptr() doing? I can't find it anywhere…

I replied about it in the other thread.

 
> > > 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.
> > 
> > Which 'other part' are you referring to? Your patch removed local_irq_save()
> > from other places as well right?
> 
> The patch converted hunks.
> 

So then there are no other local_irq_save() to match with. Or may be I did
not understand your concern, could you share any threads from past
discussions about disabling interrupts in this code? You mentioned about a
discussion from few years ago.

> > 
> >  - Joel
> > 
> > ---8<-----------------------
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 8ff71e5d0fe8b..5f49919205317 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -778,13 +778,17 @@ 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);
> > -	sdp = this_cpu_ptr(ssp->sda);
> > +	sdp = get_local_ptr(ssp->sda);
> > +	spin_lock_irqsave_rcu_node(sdp, flags);
> 
> You acquire the node lock which was not acquired before. Is that okay?
> How is get_local_ptr() different to raw_cpu_ptr()?

get_cpu_ptr() disables preemption which you might not want, right?

Most (all?) other paths are accessing the cblist under lock so I added it
here to be safe. This is anyway called from a slowpath. Do you see a problem?

thanks,

 - Joel

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

[Paul E. McKenney]

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

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

[Sebastian Andrzej Siewior]

On 2020-05-20 11:43:45 [-0700], Paul E. McKenney wrote:
> 
> 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.

Joel suggested that.

> 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.

It depends on what is required. migrate_disable() would limit you to
executing one CPU but would allow preemption. You would need a lock to
ensure exclusive access to the data structure. preempt_disable() /
local_irq_save() guarantee more than that.

Looking at the two call-sites there is no damage there is a CPU
migration after obtaining the per-CPU pointer. There could be a
CPU-migration before and after the pointer has been obtained so the code
before and after this function can not make any assumptions.

Would something like this work: ?

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -764,14 +764,15 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
 	unsigned long t;
 	unsigned long tlast;
 
+	check_init_srcu_struct(ssp);
 	/* If the local srcu_data structure has callbacks, not idle.  */
-	local_irq_save(flags);
-	sdp = this_cpu_ptr(ssp->sda);
+	sdp = raw_cpu_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
 	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
-		local_irq_restore(flags);
+		spin_unlock_irqrestore_rcu_node(sdp, flags);
 		return false; /* Callbacks already present, so not idle. */
 	}
-	local_irq_restore(flags);
+	spin_unlock_irqrestore_rcu_node(sdp, flags);
 
 	/*
 	 * No local callbacks, so probabalistically probe global state.
@@ -851,9 +852,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	}
 	rhp->func = func;
 	idx = srcu_read_lock(ssp);
-	local_irq_save(flags);
-	sdp = this_cpu_ptr(ssp->sda);
-	spin_lock_rcu_node(sdp);
+	sdp = raw_cpu_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
 	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_gp_seq));


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.

> 							Thanx, Paul

Sebastian

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

[Paul E. McKenney]

On Fri, May 22, 2020 at 05:12:55PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-20 11:43:45 [-0700], Paul E. McKenney wrote:
> > 
> > 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.
> 
> Joel suggested that.

Good!

> > 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.
> 
> It depends on what is required. migrate_disable() would limit you to
> executing one CPU but would allow preemption. You would need a lock to
> ensure exclusive access to the data structure. preempt_disable() /
> local_irq_save() guarantee more than that.
> 
> Looking at the two call-sites there is no damage there is a CPU
> migration after obtaining the per-CPU pointer. There could be a
> CPU-migration before and after the pointer has been obtained so the code
> before and after this function can not make any assumptions.
> 
> Would something like this work: ?

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.)

> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -764,14 +764,15 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
>  	unsigned long t;
>  	unsigned long tlast;
>  
> +	check_init_srcu_struct(ssp);
>  	/* If the local srcu_data structure has callbacks, not idle.  */
> -	local_irq_save(flags);
> -	sdp = this_cpu_ptr(ssp->sda);
> +	sdp = raw_cpu_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);
>  	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
> -		local_irq_restore(flags);
> +		spin_unlock_irqrestore_rcu_node(sdp, flags);
>  		return false; /* Callbacks already present, so not idle. */
>  	}
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore_rcu_node(sdp, flags);
>  
>  	/*
>  	 * No local callbacks, so probabalistically probe global state.
> @@ -851,9 +852,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>  	}
>  	rhp->func = func;
>  	idx = srcu_read_lock(ssp);
> -	local_irq_save(flags);
> -	sdp = this_cpu_ptr(ssp->sda);
> -	spin_lock_rcu_node(sdp);
> +	sdp = raw_cpu_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);
>  	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
>  	rcu_segcblist_advance(&sdp->srcu_cblist,
>  			      rcu_seq_current(&ssp->srcu_gp_seq));
> 
> 
> 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.  ;-)

 							Thanx, Paul

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

[Sebastian Andrzej Siewior]

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.

>  							Thanx, Paul

Sebastian

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

[Paul E. McKenney]

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

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

[Sebastian Andrzej Siewior]

On 2020-05-23 17:08:32 [+0200], To Paul E. McKenney 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.

|tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 32 --duration 240 --configs 8*SRCU-N
|SRCU-N ------- 2127126 GPs (147.717/s) [srcu: g26566592 f0x0 ]
|SRCU-N.2 ------- 2123520 GPs (147.467/s) [srcu: g26563696 f0x0 ]
|SRCU-N.3 ------- 2122181 GPs (147.374/s) [srcu: g26549140 f0x0 ]
|SRCU-N.4 ------- 2126099 GPs (147.646/s) [srcu: g26564232 f0x0 ]
|SRCU-N.5 ------- 2127107 GPs (147.716/s) [srcu: g26590168 f0x0 ]
|SRCU-N.6 ------- 2125489 GPs (147.603/s) [srcu: g26545616 f0x0 ]
|SRCU-N.7 ------- 2128308 GPs (147.799/s) [srcu: g26591524 f0x0 ]
|SRCU-N.8 ------- 2126432 GPs (147.669/s) [srcu: g26586816 f0x0 ]
|
|tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 32 --duration 240 --configs 8*SRCU-P
|SRCU-P ------- 565751 GPs (39.2883/s) [srcud: g5034676 f0x0 ]
|SRCU-P.2 ------- 569508 GPs (39.5492/s) [srcud: g5062872 f0x0 ]
|SRCU-P.3 ------- 574240 GPs (39.8778/s) [srcud: g5098488 f0x0 ]
|SRCU-P.4 ------- 564376 GPs (39.1928/s) [srcud: g5031244 f0x0 ]
|SRCU-P.5 ------- 563705 GPs (39.1462/s) [srcud: g5024720 f0x0 ]
|SRCU-P.6 ------- 565043 GPs (39.2391/s) [srcud: g5030272 f0x0 ]
|SRCU-P.7 ------- 567450 GPs (39.4062/s) [srcud: g5046392 f0x0 ]
|SRCU-P.8 ------- 566496 GPs (39.34/s) [srcud: g5039396 f0x0 ]

Results at
   https://breakpoint.cc/res.tar.xz

Sebastian

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

[Paul E. McKenney]

On Sun, May 24, 2020 at 09:03:56PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-05-23 17:08:32 [+0200], To Paul E. McKenney 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.
> 
> |tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 32 --duration 240 --configs 8*SRCU-N
> |SRCU-N ------- 2127126 GPs (147.717/s) [srcu: g26566592 f0x0 ]
> |SRCU-N.2 ------- 2123520 GPs (147.467/s) [srcu: g26563696 f0x0 ]
> |SRCU-N.3 ------- 2122181 GPs (147.374/s) [srcu: g26549140 f0x0 ]
> |SRCU-N.4 ------- 2126099 GPs (147.646/s) [srcu: g26564232 f0x0 ]
> |SRCU-N.5 ------- 2127107 GPs (147.716/s) [srcu: g26590168 f0x0 ]
> |SRCU-N.6 ------- 2125489 GPs (147.603/s) [srcu: g26545616 f0x0 ]
> |SRCU-N.7 ------- 2128308 GPs (147.799/s) [srcu: g26591524 f0x0 ]
> |SRCU-N.8 ------- 2126432 GPs (147.669/s) [srcu: g26586816 f0x0 ]
> |
> |tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 32 --duration 240 --configs 8*SRCU-P
> |SRCU-P ------- 565751 GPs (39.2883/s) [srcud: g5034676 f0x0 ]
> |SRCU-P.2 ------- 569508 GPs (39.5492/s) [srcud: g5062872 f0x0 ]
> |SRCU-P.3 ------- 574240 GPs (39.8778/s) [srcud: g5098488 f0x0 ]
> |SRCU-P.4 ------- 564376 GPs (39.1928/s) [srcud: g5031244 f0x0 ]
> |SRCU-P.5 ------- 563705 GPs (39.1462/s) [srcud: g5024720 f0x0 ]
> |SRCU-P.6 ------- 565043 GPs (39.2391/s) [srcud: g5030272 f0x0 ]
> |SRCU-P.7 ------- 567450 GPs (39.4062/s) [srcud: g5046392 f0x0 ]
> |SRCU-P.8 ------- 566496 GPs (39.34/s) [srcud: g5039396 f0x0 ]
> 
> Results at
>    https://breakpoint.cc/res.tar.xz

Looks good, thank you!

							Thanx, Paul

[PATCH] srcu: Avoid local_irq_save() before acquiring spinlock_t

[Sebastian Andrzej Siewior]

SRCU disables interrupts to get a stable per-CPU pointer and then
acquires the spinlock which is in the per-CPU data structure. The
release uses spin_unlock_irqrestore(). While this is correct on a non-RT
kernel, this conflicts with the RT semantics because the spinlock is
converted to a 'sleeping' spinlock. Sleeping locks can obviously not be
acquired with interrupts disabled.

Acquire the per-CPU pointer `ssp->sda' without disabling preemption and
then acquire the spinlock_t of the per-CPU data structure. The lock
will ensure that the data is consistent.
The added check_init_srcu_struct() is now needed because a statically 
defined srcu_struct may remain uninitialized until this point and the
newly introduced locking operation requires an initialized spinlock_t.

This change was tested for four hours with 8*SRCU-N and 8*SRCU-P without
causing any warnings.

Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: rcu@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/rcu/srcutree.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 0c71505f0e19c..9459bca58c380 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -764,14 +764,15 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
 	unsigned long t;
 	unsigned long tlast;
 
+	check_init_srcu_struct(ssp);
 	/* If the local srcu_data structure has callbacks, not idle.  */
-	local_irq_save(flags);
-	sdp = this_cpu_ptr(ssp->sda);
+	sdp = raw_cpu_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
 	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
-		local_irq_restore(flags);
+		spin_unlock_irqrestore_rcu_node(sdp, flags);
 		return false; /* Callbacks already present, so not idle. */
 	}
-	local_irq_restore(flags);
+	spin_unlock_irqrestore_rcu_node(sdp, flags);
 
 	/*
 	 * No local callbacks, so probabalistically probe global state.
@@ -851,9 +852,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	}
 	rhp->func = func;
 	idx = srcu_read_lock(ssp);
-	local_irq_save(flags);
-	sdp = this_cpu_ptr(ssp->sda);
-	spin_lock_rcu_node(sdp);
+	sdp = raw_cpu_ptr(ssp->sda);
+	spin_lock_irqsave_rcu_node(sdp, flags);
 	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_gp_seq));
-- 
2.27.0.rc0

Re: [PATCH] srcu: Avoid local_irq_save() before acquiring spinlock_t

[Paul E. McKenney]

On Tue, May 26, 2020 at 03:41:34PM +0200, Sebastian Andrzej Siewior wrote:
> SRCU disables interrupts to get a stable per-CPU pointer and then
> acquires the spinlock which is in the per-CPU data structure. The
> release uses spin_unlock_irqrestore(). While this is correct on a non-RT
> kernel, this conflicts with the RT semantics because the spinlock is
> converted to a 'sleeping' spinlock. Sleeping locks can obviously not be
> acquired with interrupts disabled.
> 
> Acquire the per-CPU pointer `ssp->sda' without disabling preemption and
> then acquire the spinlock_t of the per-CPU data structure. The lock
> will ensure that the data is consistent.
> The added check_init_srcu_struct() is now needed because a statically 
> defined srcu_struct may remain uninitialized until this point and the
> newly introduced locking operation requires an initialized spinlock_t.
> 
> This change was tested for four hours with 8*SRCU-N and 8*SRCU-P without
> causing any warnings.

Queued, thank you!!!

							Thanx, Paul

> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: rcu@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/rcu/srcutree.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 0c71505f0e19c..9459bca58c380 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -764,14 +764,15 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
>  	unsigned long t;
>  	unsigned long tlast;
>  
> +	check_init_srcu_struct(ssp);
>  	/* If the local srcu_data structure has callbacks, not idle.  */
> -	local_irq_save(flags);
> -	sdp = this_cpu_ptr(ssp->sda);
> +	sdp = raw_cpu_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);
>  	if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) {
> -		local_irq_restore(flags);
> +		spin_unlock_irqrestore_rcu_node(sdp, flags);
>  		return false; /* Callbacks already present, so not idle. */
>  	}
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore_rcu_node(sdp, flags);
>  
>  	/*
>  	 * No local callbacks, so probabalistically probe global state.
> @@ -851,9 +852,8 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
>  	}
>  	rhp->func = func;
>  	idx = srcu_read_lock(ssp);
> -	local_irq_save(flags);
> -	sdp = this_cpu_ptr(ssp->sda);
> -	spin_lock_rcu_node(sdp);
> +	sdp = raw_cpu_ptr(ssp->sda);
> +	spin_lock_irqsave_rcu_node(sdp, flags);
>  	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
>  	rcu_segcblist_advance(&sdp->srcu_cblist,
>  			      rcu_seq_current(&ssp->srcu_gp_seq));
> -- 
> 2.27.0.rc0
> 

Re: [PATCH] srcu: Avoid local_irq_save() before acquiring spinlock_t

[Sebastian Andrzej Siewior]

On 2020-05-26 09:16:09 [-0700], Paul E. McKenney wrote:
> Queued, thank you!!!
thank you.

> 							Thanx, Paul

Sebastian