* [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed()
@ 2025-09-09 13:39 Zqiang
2025-09-10 13:55 ` Paul E. McKenney
0 siblings, 1 reply; 8+ messages in thread
From: Zqiang @ 2025-09-09 13:39 UTC (permalink / raw)
To: paulmck, frederic, neeraj.upadhyay, joelagnelf, boqun.feng,
urezki
Cc: qiang.zhang, rcu, linux-kernel
Currently, the srcu_gp_start_if_needed() is always be invoked in
preempt disable's critical section, this commit therefore remove
redundant preempt_disable/enable() in srcu_gp_start_if_needed()
and adds a call to lockdep_assert_preemption_disabled() in order
to enable lockdep to diagnose mistaken invocations of this function
from preempts-enabled code.
Fixes: 65b4a59557f6 ("srcu: Make Tiny SRCU explicitly disable preemption")
Signed-off-by: Zqiang <qiang.zhang@linux.dev>
---
kernel/rcu/srcutiny.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
index b52ec45698e8..b2da188133fc 100644
--- a/kernel/rcu/srcutiny.c
+++ b/kernel/rcu/srcutiny.c
@@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
{
unsigned long cookie;
- preempt_disable(); // Needed for PREEMPT_LAZY
+ lockdep_assert_preemption_disabled();
cookie = get_state_synchronize_srcu(ssp);
if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) {
- preempt_enable();
return;
}
WRITE_ONCE(ssp->srcu_idx_max, cookie);
@@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp)
else if (list_empty(&ssp->srcu_work.entry))
list_add(&ssp->srcu_work.entry, &srcu_boot_list);
}
- preempt_enable();
}
/*
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-09 13:39 [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() Zqiang @ 2025-09-10 13:55 ` Paul E. McKenney 2025-09-10 14:36 ` Joel Fernandes 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2025-09-10 13:55 UTC (permalink / raw) To: Zqiang Cc: frederic, neeraj.upadhyay, joelagnelf, boqun.feng, urezki, rcu, linux-kernel On Tue, Sep 09, 2025 at 09:39:28PM +0800, Zqiang wrote: > Currently, the srcu_gp_start_if_needed() is always be invoked in > preempt disable's critical section, this commit therefore remove > redundant preempt_disable/enable() in srcu_gp_start_if_needed() > and adds a call to lockdep_assert_preemption_disabled() in order > to enable lockdep to diagnose mistaken invocations of this function > from preempts-enabled code. > > Fixes: 65b4a59557f6 ("srcu: Make Tiny SRCU explicitly disable preemption") > Signed-off-by: Zqiang <qiang.zhang@linux.dev> Very good, applied for testing and further review, thank you! Thanx, Paul > --- > kernel/rcu/srcutiny.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > index b52ec45698e8..b2da188133fc 100644 > --- a/kernel/rcu/srcutiny.c > +++ b/kernel/rcu/srcutiny.c > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > { > unsigned long cookie; > > - preempt_disable(); // Needed for PREEMPT_LAZY > + lockdep_assert_preemption_disabled(); > cookie = get_state_synchronize_srcu(ssp); > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > - preempt_enable(); > return; > } > WRITE_ONCE(ssp->srcu_idx_max, cookie); > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > else if (list_empty(&ssp->srcu_work.entry)) > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > } > - preempt_enable(); > } > > /* > -- > 2.48.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-10 13:55 ` Paul E. McKenney @ 2025-09-10 14:36 ` Joel Fernandes 2025-09-10 15:52 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Joel Fernandes @ 2025-09-10 14:36 UTC (permalink / raw) To: Paul E. McKenney Cc: Zqiang, frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel [..] > > kernel/rcu/srcutiny.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > index b52ec45698e8..b2da188133fc 100644 > > --- a/kernel/rcu/srcutiny.c > > +++ b/kernel/rcu/srcutiny.c > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > { > > unsigned long cookie; > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > + lockdep_assert_preemption_disabled(); nit: Do we still want to keep the comment that the expectation of preemption being disabled is for the LAZY case? thanks, - Joel > > cookie = get_state_synchronize_srcu(ssp); > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > - preempt_enable(); > > return; > > } > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > else if (list_empty(&ssp->srcu_work.entry)) > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > } > > - preempt_enable(); > > } > > > > /* > > -- > > 2.48.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-10 14:36 ` Joel Fernandes @ 2025-09-10 15:52 ` Paul E. McKenney 2025-09-11 0:36 ` Zqiang 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2025-09-10 15:52 UTC (permalink / raw) To: Joel Fernandes Cc: Zqiang, frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel On Wed, Sep 10, 2025 at 10:36:20AM -0400, Joel Fernandes wrote: > [..] > > > kernel/rcu/srcutiny.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > index b52ec45698e8..b2da188133fc 100644 > > > --- a/kernel/rcu/srcutiny.c > > > +++ b/kernel/rcu/srcutiny.c > > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > { > > > unsigned long cookie; > > > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > > + lockdep_assert_preemption_disabled(); > > nit: Do we still want to keep the comment that the expectation of preemption > being disabled is for the LAZY case? Good point, and I do believe that we do. Zqiang, any reason not to add this comment back in? Thanx, Paul > thanks, > > - Joel > > > > > cookie = get_state_synchronize_srcu(ssp); > > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > > - preempt_enable(); > > > return; > > > } > > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > else if (list_empty(&ssp->srcu_work.entry)) > > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > > } > > > - preempt_enable(); > > > } > > > > > > /* > > > -- > > > 2.48.1 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-10 15:52 ` Paul E. McKenney @ 2025-09-11 0:36 ` Zqiang 2025-09-11 11:37 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Zqiang @ 2025-09-11 0:36 UTC (permalink / raw) To: paulmck, Joel Fernandes Cc: frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel > > On Wed, Sep 10, 2025 at 10:36:20AM -0400, Joel Fernandes wrote: > > > > > [..] > > > kernel/rcu/srcutiny.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > index b52ec45698e8..b2da188133fc 100644 > > > --- a/kernel/rcu/srcutiny.c > > > +++ b/kernel/rcu/srcutiny.c > > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > { > > > unsigned long cookie; > > > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > > + lockdep_assert_preemption_disabled(); > > > > nit: Do we still want to keep the comment that the expectation of preemption > > being disabled is for the LAZY case? > > > Good point, and I do believe that we do. Zqiang, any reason not to > add this comment back in? in rcu-tree, this commit: (935147775c977 "EXP srcu: Enable Tiny SRCU On all CONFIG_SMP=n kernels") make preempt disable needed for CONFIG_PREEMPT=y or CONFIG_PREEMPT_LAZY=y when the CONFIG_SMP=n. do we need to replace "Needed for PREEMPT_LAZY" comments with "Needed for PREEMPT or PREEMPT_LAZY"? Thanks Zqiang > > Thanx, Paul > > > > > thanks, > > > > - Joel > > > > > > > cookie = get_state_synchronize_srcu(ssp); > > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > > - preempt_enable(); > > > return; > > > } > > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > else if (list_empty(&ssp->srcu_work.entry)) > > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > > } > > > - preempt_enable(); > > > } > > > > > > /* > > > -- > > > 2.48.1 > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-11 0:36 ` Zqiang @ 2025-09-11 11:37 ` Paul E. McKenney 2025-09-11 11:46 ` Zqiang 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2025-09-11 11:37 UTC (permalink / raw) To: Zqiang Cc: Joel Fernandes, frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel On Thu, Sep 11, 2025 at 12:36:45AM +0000, Zqiang wrote: > > > > On Wed, Sep 10, 2025 at 10:36:20AM -0400, Joel Fernandes wrote: > > > > > > > > [..] > > > > kernel/rcu/srcutiny.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > > index b52ec45698e8..b2da188133fc 100644 > > > > --- a/kernel/rcu/srcutiny.c > > > > +++ b/kernel/rcu/srcutiny.c > > > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > { > > > > unsigned long cookie; > > > > > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > > > + lockdep_assert_preemption_disabled(); > > > > > > nit: Do we still want to keep the comment that the expectation of preemption > > > being disabled is for the LAZY case? > > > > > Good point, and I do believe that we do. Zqiang, any reason not to > > add this comment back in? > > in rcu-tree, this commit: > > (935147775c977 "EXP srcu: Enable Tiny SRCU On all CONFIG_SMP=n kernels") > > make preempt disable needed for CONFIG_PREEMPT=y or CONFIG_PREEMPT_LAZY=y > when the CONFIG_SMP=n. do we need to replace "Needed for PREEMPT_LAZY" > comments with "Needed for PREEMPT or PREEMPT_LAZY"? Good point as well, thank you! And I need to decide whether I should send that patch upstream. Its original purpose was to test PREEMPT_LAZY=y better than could be tested with PREEMPT_LAZY. Thoughts? Thanx, Paul > Thanks > Zqiang > > > > > > Thanx, Paul > > > > > > > > thanks, > > > > > > - Joel > > > > > > > > > > cookie = get_state_synchronize_srcu(ssp); > > > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > > > - preempt_enable(); > > > > return; > > > > } > > > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > else if (list_empty(&ssp->srcu_work.entry)) > > > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > > > } > > > > - preempt_enable(); > > > > } > > > > > > > > /* > > > > -- > > > > 2.48.1 > > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-11 11:37 ` Paul E. McKenney @ 2025-09-11 11:46 ` Zqiang 2025-09-11 12:06 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Zqiang @ 2025-09-11 11:46 UTC (permalink / raw) To: paulmck Cc: Joel Fernandes, frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel > > On Thu, Sep 11, 2025 at 12:36:45AM +0000, Zqiang wrote: > > > > > On Wed, Sep 10, 2025 at 10:36:20AM -0400, Joel Fernandes wrote: > > > > > > > > [..] > > > > kernel/rcu/srcutiny.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > > index b52ec45698e8..b2da188133fc 100644 > > > > --- a/kernel/rcu/srcutiny.c > > > > +++ b/kernel/rcu/srcutiny.c > > > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > { > > > > unsigned long cookie; > > > > > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > > > + lockdep_assert_preemption_disabled(); > > > > > > nit: Do we still want to keep the comment that the expectation of preemption > > > being disabled is for the LAZY case? > > > > > Good point, and I do believe that we do. Zqiang, any reason not to > > add this comment back in? > > > > in rcu-tree, this commit: > > > > (935147775c977 "EXP srcu: Enable Tiny SRCU On all CONFIG_SMP=n kernels") > > > > make preempt disable needed for CONFIG_PREEMPT=y or CONFIG_PREEMPT_LAZY=y > > when the CONFIG_SMP=n. do we need to replace "Needed for PREEMPT_LAZY" > > comments with "Needed for PREEMPT or PREEMPT_LAZY"? > > > Good point as well, thank you! And I need to decide whether I should > send that patch upstream. Its original purpose was to test PREEMPT_LAZY=y > better than could be tested with PREEMPT_LAZY. > > Thoughts? I will add "Needed for PREEMPT_LAZY" comments, if this commit (935147775c977) is send to upstream, will update comments again in the future. Thanks Zqiang > > Thanx, Paul > > > > > Thanks > > Zqiang > > > > > > > > Thanx, Paul > > > > > > > > thanks, > > > > > > - Joel > > > > > > > > > > cookie = get_state_synchronize_srcu(ssp); > > > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > > > - preempt_enable(); > > > > return; > > > > } > > > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > else if (list_empty(&ssp->srcu_work.entry)) > > > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > > > } > > > > - preempt_enable(); > > > > } > > > > > > > > /* > > > > -- > > > > 2.48.1 > > > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() 2025-09-11 11:46 ` Zqiang @ 2025-09-11 12:06 ` Paul E. McKenney 0 siblings, 0 replies; 8+ messages in thread From: Paul E. McKenney @ 2025-09-11 12:06 UTC (permalink / raw) To: Zqiang Cc: Joel Fernandes, frederic, neeraj.upadhyay, boqun.feng, urezki, rcu, linux-kernel On Thu, Sep 11, 2025 at 11:46:36AM +0000, Zqiang wrote: > > > > On Thu, Sep 11, 2025 at 12:36:45AM +0000, Zqiang wrote: > > > > > > > > On Wed, Sep 10, 2025 at 10:36:20AM -0400, Joel Fernandes wrote: > > > > > > > > > > > [..] > > > > > kernel/rcu/srcutiny.c | 4 +--- > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > > > index b52ec45698e8..b2da188133fc 100644 > > > > > --- a/kernel/rcu/srcutiny.c > > > > > +++ b/kernel/rcu/srcutiny.c > > > > > @@ -181,10 +181,9 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > > { > > > > > unsigned long cookie; > > > > > > > > > > - preempt_disable(); // Needed for PREEMPT_LAZY > > > > > + lockdep_assert_preemption_disabled(); > > > > > > > > nit: Do we still want to keep the comment that the expectation of preemption > > > > being disabled is for the LAZY case? > > > > > > > Good point, and I do believe that we do. Zqiang, any reason not to > > > add this comment back in? > > > > > > in rcu-tree, this commit: > > > > > > (935147775c977 "EXP srcu: Enable Tiny SRCU On all CONFIG_SMP=n kernels") > > > > > > make preempt disable needed for CONFIG_PREEMPT=y or CONFIG_PREEMPT_LAZY=y > > > when the CONFIG_SMP=n. do we need to replace "Needed for PREEMPT_LAZY" > > > comments with "Needed for PREEMPT or PREEMPT_LAZY"? > > > > > Good point as well, thank you! And I need to decide whether I should > > send that patch upstream. Its original purpose was to test PREEMPT_LAZY=y > > better than could be tested with PREEMPT_LAZY. > > > > Thoughts? > > I will add "Needed for PREEMPT_LAZY" comments, if this commit (935147775c977) is > send to upstream, will update comments again in the future. That sounds good to me, thank you! Thanx, Paul > Thanks > Zqiang > > > > > Thanx, Paul > > > > > > > > Thanks > > > Zqiang > > > > > > > > > > > > Thanx, Paul > > > > > > > > > > > thanks, > > > > > > > > - Joel > > > > > > > > > > > > > cookie = get_state_synchronize_srcu(ssp); > > > > > if (ULONG_CMP_GE(READ_ONCE(ssp->srcu_idx_max), cookie)) { > > > > > - preempt_enable(); > > > > > return; > > > > > } > > > > > WRITE_ONCE(ssp->srcu_idx_max, cookie); > > > > > @@ -194,7 +193,6 @@ static void srcu_gp_start_if_needed(struct srcu_struct *ssp) > > > > > else if (list_empty(&ssp->srcu_work.entry)) > > > > > list_add(&ssp->srcu_work.entry, &srcu_boot_list); > > > > > } > > > > > - preempt_enable(); > > > > > } > > > > > > > > > > /* > > > > > -- > > > > > 2.48.1 > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-09-11 12:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-09 13:39 [PATCH v2] srcu/tiny: Remove preempt_disable/enable() in srcu_gp_start_if_needed() Zqiang 2025-09-10 13:55 ` Paul E. McKenney 2025-09-10 14:36 ` Joel Fernandes 2025-09-10 15:52 ` Paul E. McKenney 2025-09-11 0:36 ` Zqiang 2025-09-11 11:37 ` Paul E. McKenney 2025-09-11 11:46 ` Zqiang 2025-09-11 12:06 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).