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);
}
/**
