Hello,
This series contains SRCU updates:
1. Remove superfluous callbacks advancing from srcu_gp_start(),
courtesy of Frederic Weisbecker.
2. No need to advance/accelerate if no callback enqueued, courtesy
of Frederic Weisbecker.
3. Explain why callbacks invocations can't run concurrently,
courtesy of Frederic Weisbecker.
Thanks
Neeraj
------------------------------------------------------------------------
b/kernel/rcu/srcutree.c | 10 ----------
kernel/rcu/srcutree.c | 14 +++++++++++---
2 files changed, 11 insertions(+), 13 deletions(-)
From: Frederic Weisbecker <[email protected]>
Callbacks advancing on SRCU must be performed on two specific places:
1) On enqueue time in order to make room for the acceleration of the
new callback.
2) On invocation time in order to move the callbacks ready to invoke.
Any other callback advancing callsite is needless. Remove the remaining
one in srcu_gp_start().
Co-developed-by: Yong He <[email protected]>
Signed-off-by: Yong He <[email protected]>
Co-developed-by: Joel Fernandes <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Co-developed-by: Neeraj Upadhyay (AMD) <[email protected]>
Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
---
kernel/rcu/srcutree.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 560e99ec5333..e9356a103626 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -772,20 +772,10 @@ EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
*/
static void srcu_gp_start(struct srcu_struct *ssp)
{
- struct srcu_data *sdp;
int state;
- if (smp_load_acquire(&ssp->srcu_sup->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
- sdp = per_cpu_ptr(ssp->sda, get_boot_cpu_id());
- else
- sdp = this_cpu_ptr(ssp->sda);
lockdep_assert_held(&ACCESS_PRIVATE(ssp->srcu_sup, lock));
WARN_ON_ONCE(ULONG_CMP_GE(ssp->srcu_sup->srcu_gp_seq, ssp->srcu_sup->srcu_gp_seq_needed));
- spin_lock_rcu_node(sdp); /* Interrupts already disabled. */
- rcu_segcblist_advance(&sdp->srcu_cblist,
- rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
- WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
- spin_unlock_rcu_node(sdp); /* Interrupts remain disabled. */
WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
--
2.40.1
From: Frederic Weisbecker <[email protected]>
If an SRCU barrier is queued while callbacks are running and a new
callbacks invocator for the same sdp were to run concurrently, the
RCU barrier might execute too early. As this requirement is non-obvious,
make sure to keep a record.
Signed-off-by: Frederic Weisbecker <[email protected]>
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
---
kernel/rcu/srcutree.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 2bfc8ed1eed2..0351a4e83529 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
rcu_segcblist_advance(&sdp->srcu_cblist,
rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+ /*
+ * Although this function is theoretically re-entrant, concurrent
+ * callbacks invocation is disallowed to avoid executing an SRCU barrier
+ * too early.
+ */
if (sdp->srcu_cblist_invoking ||
!rcu_segcblist_ready_cbs(&sdp->srcu_cblist)) {
spin_unlock_irq_rcu_node(sdp);
@@ -1745,6 +1750,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
sdp->srcu_cblist_invoking = false;
more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
spin_unlock_irq_rcu_node(sdp);
+ /* An SRCU barrier or callbacks from previous nesting work pending */
if (more)
srcu_schedule_cbs_sdp(sdp, 0);
}
--
2.40.1
From: Frederic Weisbecker <[email protected]>
While in grace period start, there is nothing to accelerate and
therefore no need to advance the callbacks either if no callback is
to be enqueued.
Spare these needless operations in this case.
Signed-off-by: Frederic Weisbecker <[email protected]>
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
---
kernel/rcu/srcutree.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index e9356a103626..2bfc8ed1eed2 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1261,9 +1261,11 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
* period (gp_num = X + 8). So acceleration fails.
*/
s = rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq);
- rcu_segcblist_advance(&sdp->srcu_cblist,
- rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
- WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s) && rhp);
+ if (rhp) {
+ rcu_segcblist_advance(&sdp->srcu_cblist,
+ rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
+ WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s));
+ }
if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {
sdp->srcu_gp_seq_needed = s;
needgp = true;
--
2.40.1
On Tue, Dec 12, 2023 at 12:48 PM Neeraj Upadhyay (AMD)
<[email protected]> wrote:
>
> From: Frederic Weisbecker <[email protected]>
>
> If an SRCU barrier is queued while callbacks are running and a new
> callbacks invocator for the same sdp were to run concurrently, the
> RCU barrier might execute too early. As this requirement is non-obvious,
> make sure to keep a record.
>
> Signed-off-by: Frederic Weisbecker <[email protected]>
> Reviewed-by: Joel Fernandes (Google) <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
> ---
> kernel/rcu/srcutree.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 2bfc8ed1eed2..0351a4e83529 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> rcu_segcblist_advance(&sdp->srcu_cblist,
> rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> + /*
> + * Although this function is theoretically re-entrant, concurrent
> + * callbacks invocation is disallowed to avoid executing an SRCU barrier
> + * too early.
> + */
Side comment:
I guess even without the barrier reasoning, it is best not to allow
concurrent CB execution anyway since it diverges from the behavior of
straight RCU :)
- Joel
On Wed, Dec 13, 2023 at 09:27:09AM -0500, Joel Fernandes wrote:
> On Tue, Dec 12, 2023 at 12:48 PM Neeraj Upadhyay (AMD)
> <[email protected]> wrote:
> >
> > From: Frederic Weisbecker <[email protected]>
> >
> > If an SRCU barrier is queued while callbacks are running and a new
> > callbacks invocator for the same sdp were to run concurrently, the
> > RCU barrier might execute too early. As this requirement is non-obvious,
> > make sure to keep a record.
> >
> > Signed-off-by: Frederic Weisbecker <[email protected]>
> > Reviewed-by: Joel Fernandes (Google) <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
> > ---
> > kernel/rcu/srcutree.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 2bfc8ed1eed2..0351a4e83529 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> > rcu_segcblist_advance(&sdp->srcu_cblist,
> > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > + /*
> > + * Although this function is theoretically re-entrant, concurrent
> > + * callbacks invocation is disallowed to avoid executing an SRCU barrier
> > + * too early.
> > + */
>
> Side comment:
> I guess even without the barrier reasoning, it is best not to allow
> concurrent CB execution anyway since it diverges from the behavior of
> straight RCU :)
Good point!
But please do not forget item 12 on the list in checklist.rst. ;-)
(Which I just updated to include the other call_rcu*() functions.)
Thanx, Paul
On Wed, Dec 13, 2023 at 12:52 PM Paul E. McKenney <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 09:27:09AM -0500, Joel Fernandes wrote:
> > On Tue, Dec 12, 2023 at 12:48 PM Neeraj Upadhyay (AMD)
> > <[email protected]> wrote:
> > >
> > > From: Frederic Weisbecker <[email protected]>
> > >
> > > If an SRCU barrier is queued while callbacks are running and a new
> > > callbacks invocator for the same sdp were to run concurrently, the
> > > RCU barrier might execute too early. As this requirement is non-obvious,
> > > make sure to keep a record.
> > >
> > > Signed-off-by: Frederic Weisbecker <[email protected]>
> > > Reviewed-by: Joel Fernandes (Google) <[email protected]>
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
> > > ---
> > > kernel/rcu/srcutree.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 2bfc8ed1eed2..0351a4e83529 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > > WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > + /*
> > > + * Although this function is theoretically re-entrant, concurrent
> > > + * callbacks invocation is disallowed to avoid executing an SRCU barrier
> > > + * too early.
> > > + */
> >
> > Side comment:
> > I guess even without the barrier reasoning, it is best not to allow
> > concurrent CB execution anyway since it diverges from the behavior of
> > straight RCU :)
>
> Good point!
>
> But please do not forget item 12 on the list in checklist.rst. ;-)
> (Which I just updated to include the other call_rcu*() functions.)
I think this is more so now with recent kernels (with the dynamic nocb
switch) than with older kernels right? I haven't kept up with the
checklist recently (which is my bad).
My understanding comes from the fact that the RCU barrier depends on
callbacks on the same CPU executing in order with straight RCU
otherwise it breaks. Hence my comment. But as you pointed out, that's
outdated knowledge.
I should just shut up and hide in shame now.
:-/
- Joel
From: Sebastian Andrzej Siewior <[email protected]>
It is claimed that srcu_read_lock_nmisafe() NMI-safe. However it
triggers a lockdep if used from NMI because lockdep expects a deadlock
since nothing disables NMIs while the lock is acquired.
This is because commit f0f44752f5f61 ("rcu: Annotate SRCU's update-side
lockdep dependencies") annotates synchronize_srcu() as a write lock
usage. This helps to detect a deadlocks such as
srcu_read_lock();
synchronize_srcu();
srcu_read_unlock();
The side effect is that the lock srcu_struct now has a USED usage in normal
contexts, so it conflicts with a USED_READ usage in NMI. But this shouldn't
cause a real deadlock because the write lock usage from synchronize_srcu()
is a fake one and only used for read/write deadlock detection.
Use a try-lock annotation for srcu_read_lock_nmisafe() to avoid lockdep
complains if used from NMI.
Fixes: f0f44752f5f6 ("rcu: Annotate SRCU's update-side lockdep dependencies")
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Boqun Feng <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
---
include/linux/rcupdate.h | 6 ++++++
include/linux/srcu.h | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f7206b2623c9..31d523c4e089 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -301,6 +301,11 @@ static inline void rcu_lock_acquire(struct lockdep_map *map)
lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
}
+static inline void rcu_try_lock_acquire(struct lockdep_map *map)
+{
+ lock_acquire(map, 0, 1, 2, 0, NULL, _THIS_IP_);
+}
+
static inline void rcu_lock_release(struct lockdep_map *map)
{
lock_release(map, _THIS_IP_);
@@ -315,6 +320,7 @@ int rcu_read_lock_any_held(void);
#else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
# define rcu_lock_acquire(a) do { } while (0)
+# define rcu_try_lock_acquire(a) do { } while (0)
# define rcu_lock_release(a) do { } while (0)
static inline int rcu_read_lock_held(void)
diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 127ef3b2e607..236610e4a8fa 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -229,7 +229,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
srcu_check_nmi_safety(ssp, true);
retval = __srcu_read_lock_nmisafe(ssp);
- rcu_lock_acquire(&ssp->dep_map);
+ rcu_try_lock_acquire(&ssp->dep_map);
return retval;
}
--
2.40.1
On Wed, Dec 13, 2023 at 01:35:22PM -0500, Joel Fernandes wrote:
> On Wed, Dec 13, 2023 at 12:52 PM Paul E. McKenney <[email protected]> wrote:
> >
> > On Wed, Dec 13, 2023 at 09:27:09AM -0500, Joel Fernandes wrote:
> > > On Tue, Dec 12, 2023 at 12:48 PM Neeraj Upadhyay (AMD)
> > > <[email protected]> wrote:
> > > >
> > > > From: Frederic Weisbecker <[email protected]>
> > > >
> > > > If an SRCU barrier is queued while callbacks are running and a new
> > > > callbacks invocator for the same sdp were to run concurrently, the
> > > > RCU barrier might execute too early. As this requirement is non-obvious,
> > > > make sure to keep a record.
> > > >
> > > > Signed-off-by: Frederic Weisbecker <[email protected]>
> > > > Reviewed-by: Joel Fernandes (Google) <[email protected]>
> > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
> > > > ---
> > > > kernel/rcu/srcutree.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > index 2bfc8ed1eed2..0351a4e83529 100644
> > > > --- a/kernel/rcu/srcutree.c
> > > > +++ b/kernel/rcu/srcutree.c
> > > > @@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > > > WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> > > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > > + /*
> > > > + * Although this function is theoretically re-entrant, concurrent
> > > > + * callbacks invocation is disallowed to avoid executing an SRCU barrier
> > > > + * too early.
> > > > + */
> > >
> > > Side comment:
> > > I guess even without the barrier reasoning, it is best not to allow
> > > concurrent CB execution anyway since it diverges from the behavior of
> > > straight RCU :)
> >
> > Good point!
> >
> > But please do not forget item 12 on the list in checklist.rst. ;-)
> > (Which I just updated to include the other call_rcu*() functions.)
>
> I think this is more so now with recent kernels (with the dynamic nocb
> switch) than with older kernels right? I haven't kept up with the
> checklist recently (which is my bad).
You are quite correct! But even before this, I was saying that
lack of same-CPU callback concurrency was an accident of the current
implementation rather than a guarantee. For example, there might come
a time when RCU needs to respond to callback flooding with concurrent
execution of the flooded CPU's callbacks. Or not, but we do need to
keep this option open.
> My understanding comes from the fact that the RCU barrier depends on
> callbacks on the same CPU executing in order with straight RCU
> otherwise it breaks. Hence my comment. But as you pointed out, that's
> outdated knowledge.
That is still one motivation for ordered execution of callbacks. For the
dynamic nocb switch, we could have chosen to make rcu_barrier() place
a callback on both lists, but we instead chose to exclude rcu_barrier()
calls during the switch.
> I should just shut up and hide in shame now.
No need for that! After all, one motivation for Requirements.rst was
to help me keep track of all this stuff.
Thanx, Paul
> :-/
>
> - Joel
On Wed, Dec 13, 2023 at 1:55 PM Paul E. McKenney <[email protected]> wrote:
>
> On Wed, Dec 13, 2023 at 01:35:22PM -0500, Joel Fernandes wrote:
> > On Wed, Dec 13, 2023 at 12:52 PM Paul E. McKenney <[email protected]> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 09:27:09AM -0500, Joel Fernandes wrote:
> > > > On Tue, Dec 12, 2023 at 12:48 PM Neeraj Upadhyay (AMD)
> > > > <[email protected]> wrote:
> > > > >
> > > > > From: Frederic Weisbecker <[email protected]>
> > > > >
> > > > > If an SRCU barrier is queued while callbacks are running and a new
> > > > > callbacks invocator for the same sdp were to run concurrently, the
> > > > > RCU barrier might execute too early. As this requirement is non-obvious,
> > > > > make sure to keep a record.
> > > > >
> > > > > Signed-off-by: Frederic Weisbecker <[email protected]>
> > > > > Reviewed-by: Joel Fernandes (Google) <[email protected]>
> > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > > Signed-off-by: Neeraj Upadhyay (AMD) <[email protected]>
> > > > > ---
> > > > > kernel/rcu/srcutree.c | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > > > index 2bfc8ed1eed2..0351a4e83529 100644
> > > > > --- a/kernel/rcu/srcutree.c
> > > > > +++ b/kernel/rcu/srcutree.c
> > > > > @@ -1715,6 +1715,11 @@ static void srcu_invoke_callbacks(struct work_struct *work)
> > > > > WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
> > > > > rcu_segcblist_advance(&sdp->srcu_cblist,
> > > > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> > > > > + /*
> > > > > + * Although this function is theoretically re-entrant, concurrent
> > > > > + * callbacks invocation is disallowed to avoid executing an SRCU barrier
> > > > > + * too early.
> > > > > + */
> > > >
> > > > Side comment:
> > > > I guess even without the barrier reasoning, it is best not to allow
> > > > concurrent CB execution anyway since it diverges from the behavior of
> > > > straight RCU :)
> > >
> > > Good point!
> > >
> > > But please do not forget item 12 on the list in checklist.rst. ;-)
> > > (Which I just updated to include the other call_rcu*() functions.)
> >
> > I think this is more so now with recent kernels (with the dynamic nocb
> > switch) than with older kernels right? I haven't kept up with the
> > checklist recently (which is my bad).
>
> You are quite correct! But even before this, I was saying that
> lack of same-CPU callback concurrency was an accident of the current
> implementation rather than a guarantee. For example, there might come
> a time when RCU needs to respond to callback flooding with concurrent
> execution of the flooded CPU's callbacks. Or not, but we do need to
> keep this option open.
Got it, reminds me to focus on requirements as well along with implementation.
> > My understanding comes from the fact that the RCU barrier depends on
> > callbacks on the same CPU executing in order with straight RCU
> > otherwise it breaks. Hence my comment. But as you pointed out, that's
> > outdated knowledge.
>
> That is still one motivation for ordered execution of callbacks. For the
> dynamic nocb switch, we could have chosen to make rcu_barrier() place
> a callback on both lists, but we instead chose to exclude rcu_barrier()
> calls during the switch.
Right!
> > I should just shut up and hide in shame now.
>
> No need for that! After all, one motivation for Requirements.rst was
> to help me keep track of all this stuff.
Thanks!
- Joel