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 <[email protected]>
---
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;
/* 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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Fri, Jul 7, 2023 at 1:16 PM Joel Fernandes <[email protected]> wrote:
> >
> > On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
> > > > > > ---
> > > > > > 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
> > > > > >