| Message ID | 20230704082615.7415-1-qiang.zhang1211@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcu: Make srcu_might_be_idle() take early return if rcu_gp_is_normal() return true | expand |
On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > CPU's sdp->lock will be acquired to check whether there are pending > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > probabilistically probe global state to decide whether to convert to > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > kernels and after the rcu_set_runtime_mode() is called, invoke the > rcu_gp_is_normal() is always return true, this mean that invoke the > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > probe global state in srcu_might_be_idle(). > > This commit therefore make srcu_might_be_idle() return immediately if the > rcu_gp_is_normal() return true. > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > --- > kernel/rcu/srcutree.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 20d7a238d675..aea49cb60a45 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > check_init_srcu_struct(ssp); > + if (rcu_gp_is_normal()) > + return false; Again, thank you for looking into SRCU! I am not at all enthusiastic about this one. With this change, the name srcu_might_be_idle() is no longer accurate. Yes, the name could change, but any name would be longer and more confusing. So unless there is a measureable benefit to this one on a production workload, I cannot justify taking it. Is there a measureable benefit? Thanx, Paul > /* If the local srcu_data structure has callbacks, not idle. */ > sdp = raw_cpu_ptr(ssp->sda); > spin_lock_irqsave_rcu_node(sdp, flags); > -- > 2.17.1 >
> > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > CPU's sdp->lock will be acquired to check whether there are pending > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > probabilistically probe global state to decide whether to convert to > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > rcu_gp_is_normal() is always return true, this mean that invoke the > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > probe global state in srcu_might_be_idle(). > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > rcu_gp_is_normal() return true. > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > --- > > kernel/rcu/srcutree.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 20d7a238d675..aea49cb60a45 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > unsigned long tlast; > > > > check_init_srcu_struct(ssp); > > + if (rcu_gp_is_normal()) > > + return false; > > Again, thank you for looking into SRCU! > > I am not at all enthusiastic about this one. With this change, the name > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > but any name would be longer and more confusing. > > So unless there is a measureable benefit to this one on a production > workload, I cannot justify taking it. > > Is there a measureable benefit? > Hi, Paul I only find that for Preempt-RT kernel, the rcu_normal_after_boot is set by default: static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); This affects only rcu but also srcu, this make the synchronize_srcu() and synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), this means that call the srcu_might_be_idle() is meaningless. Thanks Zqiang > > Thanx, Paul > > > /* If the local srcu_data structure has callbacks, not idle. */ > > sdp = raw_cpu_ptr(ssp->sda); > > spin_lock_irqsave_rcu_node(sdp, flags); > > -- > > 2.17.1 > >
On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > CPU's sdp->lock will be acquired to check whether there are pending > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > probabilistically probe global state to decide whether to convert to > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > probe global state in srcu_might_be_idle(). > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > rcu_gp_is_normal() return true. > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > --- > > > kernel/rcu/srcutree.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > index 20d7a238d675..aea49cb60a45 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > unsigned long tlast; > > > > > > check_init_srcu_struct(ssp); > > > + if (rcu_gp_is_normal()) > > > + return false; > > > > Again, thank you for looking into SRCU! > > > > I am not at all enthusiastic about this one. With this change, the name > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > but any name would be longer and more confusing. > > > > So unless there is a measureable benefit to this one on a production > > workload, I cannot justify taking it. > > > > Is there a measureable benefit? > > Hi, Paul > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > set by default: > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > This affects only rcu but also srcu, this make the synchronize_srcu() and > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > this means that call the srcu_might_be_idle() is meaningless. I do understand that the current setup favors default kernel builds at runtime by a few low-cost instructions, and that your change favors, as you say, kernels built for real-time, kernels built for certain types of HPC workloads, and all kernels during a small time during boot. My question is instead whether any of this makes a measureable difference at the system level. My guess is "no, not even close", but the way to convince me otherwise would be to actually run the workload and kernels on real hardware and provide measurements showing a statistically significant difference that the workload(s) in question care(s) about. So what can you show me? And srcu_might_be_idle() is not meaningless in that situation, just ignored completely. And that is in fact the nature and purpose of the C-language || operator. ;-) Thanx, Paul > Thanks > Zqiang > > > > > Thanx, Paul > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > sdp = raw_cpu_ptr(ssp->sda); > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > -- > > > 2.17.1 > > >
On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > probabilistically probe global state to decide whether to convert to > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > probe global state in srcu_might_be_idle(). > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > rcu_gp_is_normal() return true. > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/srcutree.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > unsigned long tlast; > > > > > > > > check_init_srcu_struct(ssp); > > > > + if (rcu_gp_is_normal()) > > > > + return false; > > > > > > Again, thank you for looking into SRCU! > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > but any name would be longer and more confusing. > > > > > > So unless there is a measureable benefit to this one on a production > > > workload, I cannot justify taking it. > > > > > > Is there a measureable benefit? > > > > Hi, Paul > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > set by default: > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > this means that call the srcu_might_be_idle() is meaningless. > > I do understand that the current setup favors default kernel builds at > runtime by a few low-cost instructions, and that your change favors, > as you say, kernels built for real-time, kernels built for certain types > of HPC workloads, and all kernels during a small time during boot. > > My question is instead whether any of this makes a measureable difference > at the system level. > > My guess is "no, not even close", but the way to convince me otherwise > would be to actually run the workload and kernels on real hardware and > provide measurements showing a statistically significant difference that > the workload(s) in question care(s) about. > > So what can you show me? > > And srcu_might_be_idle() is not meaningless in that situation, just > ignored completely. And that is in fact the nature and purpose of the > C-language || operator. ;-) I agree with Paul, without any evidence of improvement, optimizing an obvious slow path is a NAK. thanks, - Joel
On Fri, Jul 7, 2023 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > probabilistically probe global state to decide whether to convert to > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > --- > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > unsigned long tlast; > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > + if (rcu_gp_is_normal()) > > > > > + return false; > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > but any name would be longer and more confusing. > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > workload, I cannot justify taking it. > > > > > > > > Is there a measureable benefit? > > > > > > Hi, Paul > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > set by default: > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > this means that call the srcu_might_be_idle() is meaningless. > > > > I do understand that the current setup favors default kernel builds at > > runtime by a few low-cost instructions, and that your change favors, > > as you say, kernels built for real-time, kernels built for certain types > > of HPC workloads, and all kernels during a small time during boot. > > > > My question is instead whether any of this makes a measureable difference > > at the system level. > > > > My guess is "no, not even close", but the way to convince me otherwise > > would be to actually run the workload and kernels on real hardware and > > provide measurements showing a statistically significant difference that > > the workload(s) in question care(s) about. > > > > So what can you show me? > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > ignored completely. And that is in fact the nature and purpose of the > > C-language || operator. ;-) > > I agree with Paul, without any evidence of improvement, optimizing an > obvious slow path is a NAK. Just to clarify, when I meant improvement I meant any kind (ex. better for maintenance, better performance numbers etc.). In this case, the extra 2 lines does not seem to buy much AFAICS. Thanks.
> > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > probabilistically probe global state to decide whether to convert to > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > probe global state in srcu_might_be_idle(). > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > rcu_gp_is_normal() return true. > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/srcutree.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > unsigned long tlast; > > > > > > > > check_init_srcu_struct(ssp); > > > > + if (rcu_gp_is_normal()) > > > > + return false; > > > > > > Again, thank you for looking into SRCU! > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > but any name would be longer and more confusing. > > > > > > So unless there is a measureable benefit to this one on a production > > > workload, I cannot justify taking it. > > > > > > Is there a measureable benefit? > > > > Hi, Paul > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > set by default: > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > this means that call the srcu_might_be_idle() is meaningless. > > I do understand that the current setup favors default kernel builds at > runtime by a few low-cost instructions, and that your change favors, > as you say, kernels built for real-time, kernels built for certain types > of HPC workloads, and all kernels during a small time during boot. > > My question is instead whether any of this makes a measureable difference > at the system level. > > My guess is "no, not even close", but the way to convince me otherwise > would be to actually run the workload and kernels on real hardware and > provide measurements showing a statistically significant difference that > the workload(s) in question care(s) about. > > So what can you show me? > > And srcu_might_be_idle() is not meaningless in that situation, just > ignored completely. And that is in fact the nature and purpose of the > C-language || operator. ;-) > Agree with you :) This make me want to ask another question, why srcu also use rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited srcu grace-period or only use normal grace-period instead of generating srcu_normal and srcu_expedited? Thanks Zqiang > > Thanx, Paul > > > Thanks > > Zqiang > > > > > > > > Thanx, Paul > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > -- > > > > 2.17.1 > > > >
On Sat, Jul 8, 2023 at 1:18 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Jul 7, 2023 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote: > > > > On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > > probabilistically probe global state to decide whether to convert to > > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > > unsigned long tlast; > > > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > > + if (rcu_gp_is_normal()) > > > > > > + return false; > > > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > > but any name would be longer and more confusing. > > > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > > workload, I cannot justify taking it. > > > > > > > > > > Is there a measureable benefit? > > > > > > > > Hi, Paul > > > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > > set by default: > > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > > this means that call the srcu_might_be_idle() is meaningless. > > > > > > I do understand that the current setup favors default kernel builds at > > > runtime by a few low-cost instructions, and that your change favors, > > > as you say, kernels built for real-time, kernels built for certain types > > > of HPC workloads, and all kernels during a small time during boot. > > > > > > My question is instead whether any of this makes a measureable difference > > > at the system level. > > > > > > My guess is "no, not even close", but the way to convince me otherwise > > > would be to actually run the workload and kernels on real hardware and > > > provide measurements showing a statistically significant difference that > > > the workload(s) in question care(s) about. > > > > > > So what can you show me? > > > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > > ignored completely. And that is in fact the nature and purpose of the > > > C-language || operator. ;-) > > > > I agree with Paul, without any evidence of improvement, optimizing an > > obvious slow path is a NAK. > > Just to clarify, when I meant improvement I meant any kind (ex. better > for maintenance, better performance numbers etc.). In this case, the > extra 2 lines does not seem to buy much AFAICS. > Agree, optimization does require performance data. Thanks Zqiang > > Thanks.
On Sat, Jul 08, 2023 at 10:11:30AM +0800, Z qiang wrote: > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > probabilistically probe global state to decide whether to convert to > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > --- > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > unsigned long tlast; > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > + if (rcu_gp_is_normal()) > > > > > + return false; > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > but any name would be longer and more confusing. > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > workload, I cannot justify taking it. > > > > > > > > Is there a measureable benefit? > > > > > > Hi, Paul > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > set by default: > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > this means that call the srcu_might_be_idle() is meaningless. > > > > I do understand that the current setup favors default kernel builds at > > runtime by a few low-cost instructions, and that your change favors, > > as you say, kernels built for real-time, kernels built for certain types > > of HPC workloads, and all kernels during a small time during boot. > > > > My question is instead whether any of this makes a measureable difference > > at the system level. > > > > My guess is "no, not even close", but the way to convince me otherwise > > would be to actually run the workload and kernels on real hardware and > > provide measurements showing a statistically significant difference that > > the workload(s) in question care(s) about. > > > > So what can you show me? > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > ignored completely. And that is in fact the nature and purpose of the > > C-language || operator. ;-) > > Agree with you :) > This make me want to ask another question, why srcu also use > rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited > srcu grace-period or only use normal grace-period instead of > generating srcu_normal and srcu_expedited? Because I have not yet come across a situation where it was useful for the one to be expedited and the other normal. But if you know of such a situation, let's talk about it. Thanx, Paul > Thanks > Zqiang > > > > > Thanx, Paul > > > > > Thanks > > > Zqiang > > > > > > > > > > > Thanx, Paul > > > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > > -- > > > > > 2.17.1 > > > > >
> > On Sat, Jul 08, 2023 at 10:11:30AM +0800, Z qiang wrote: > > > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > > probabilistically probe global state to decide whether to convert to > > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > > unsigned long tlast; > > > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > > + if (rcu_gp_is_normal()) > > > > > > + return false; > > > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > > but any name would be longer and more confusing. > > > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > > workload, I cannot justify taking it. > > > > > > > > > > Is there a measureable benefit? > > > > > > > > Hi, Paul > > > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > > set by default: > > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > > this means that call the srcu_might_be_idle() is meaningless. > > > > > > I do understand that the current setup favors default kernel builds at > > > runtime by a few low-cost instructions, and that your change favors, > > > as you say, kernels built for real-time, kernels built for certain types > > > of HPC workloads, and all kernels during a small time during boot. > > > > > > My question is instead whether any of this makes a measureable difference > > > at the system level. > > > > > > My guess is "no, not even close", but the way to convince me otherwise > > > would be to actually run the workload and kernels on real hardware and > > > provide measurements showing a statistically significant difference that > > > the workload(s) in question care(s) about. > > > > > > So what can you show me? > > > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > > ignored completely. And that is in fact the nature and purpose of the > > > C-language || operator. ;-) > > > > Agree with you :) > > This make me want to ask another question, why srcu also use > > rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited > > srcu grace-period or only use normal grace-period instead of > > generating srcu_normal and srcu_expedited? > > Because I have not yet come across a situation where it was useful for > the one to be expedited and the other normal. > > But if you know of such a situation, let's talk about it. > Thanks, I will let you know if I come across such a situation :) . Thanks Zqiang > > Thanx, Paul > > > Thanks > > Zqiang > > > > > > > > Thanx, Paul > > > > > > > Thanks > > > > Zqiang > > > > > > > > > > > > > > Thanx, Paul > > > > > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > > > -- > > > > > > 2.17.1 > > > > > >
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 20d7a238d675..aea49cb60a45 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) unsigned long tlast; check_init_srcu_struct(ssp); + if (rcu_gp_is_normal()) + return false; /* If the local srcu_data structure has callbacks, not idle. */ sdp = raw_cpu_ptr(ssp->sda); spin_lock_irqsave_rcu_node(sdp, flags);
When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current CPU's sdp->lock will be acquired to check whether there are pending callbacks in the sdp->srcu_cblist, if there are no pending callbacks, probabilistically probe global state to decide whether to convert to synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 kernels and after the rcu_set_runtime_mode() is called, invoke the rcu_gp_is_normal() is always return true, this mean that invoke the synchronize_srcu_expedited() always fall back to synchronize_srcu(), so there is no need to acquire sdp->lock to check sdp->srcu_cblist and probe global state in srcu_might_be_idle(). This commit therefore make srcu_might_be_idle() return immediately if the rcu_gp_is_normal() return true. Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> --- kernel/rcu/srcutree.c | 2 ++ 1 file changed, 2 insertions(+)