linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rcu: simplify the calculation of rcu_state.ncpus
@ 2020-04-17 21:39 Wei Yang
  2020-04-18  3:19 ` Paul E. McKenney
  0 siblings, 1 reply; 5+ messages in thread
From: Wei Yang @ 2020-04-17 21:39 UTC (permalink / raw)
  To: paulmck, josh; +Cc: rcu, linux-kernel, Wei Yang

There is only 1 bit set in mask, which means the difference between
oldmask and the new one would be at the position where the bit is set in
mask.

Based on this knowledge, rcu_state.ncpus could be calculated by checking
whether mask is already set in oldmask.

BTW, the comment at the last of this line is mysterious. Not sure it
could be removed or not.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 kernel/rcu/tree.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d91c9156fab2..f0d9251fa663 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3364,7 +3364,6 @@ void rcu_cpu_starting(unsigned int cpu)
 {
 	unsigned long flags;
 	unsigned long mask;
-	int nbits;
 	unsigned long oldmask;
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
@@ -3381,10 +3380,9 @@ void rcu_cpu_starting(unsigned int cpu)
 	rnp->qsmaskinitnext |= mask;
 	oldmask = rnp->expmaskinitnext;
 	rnp->expmaskinitnext |= mask;
-	oldmask ^= rnp->expmaskinitnext;
-	nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
 	/* Allow lockless access for expedited grace periods. */
-	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
+	if (!(oldmask & mask))
+		smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + 1); /* ^^^ */
 	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
 	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
 	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcu: simplify the calculation of rcu_state.ncpus
  2020-04-17 21:39 [PATCH] rcu: simplify the calculation of rcu_state.ncpus Wei Yang
@ 2020-04-18  3:19 ` Paul E. McKenney
  2020-04-18 22:02   ` Wei Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Paul E. McKenney @ 2020-04-18  3:19 UTC (permalink / raw)
  To: Wei Yang; +Cc: josh, rcu, linux-kernel

On Fri, Apr 17, 2020 at 09:39:51PM +0000, Wei Yang wrote:
> There is only 1 bit set in mask, which means the difference between
> oldmask and the new one would be at the position where the bit is set in
> mask.
> 
> Based on this knowledge, rcu_state.ncpus could be calculated by checking
> whether mask is already set in oldmask.

Nice!!!  Good eyes!

> BTW, the comment at the last of this line is mysterious. Not sure it
> could be removed or not.

The "^^^" in that comment says to look at the comment on the preceding
line.  Memory-ordering functions like smp_store_release() are supposed
to have comments indicating what they are ordering.  ;-)

Could you please do the following things and resubmit?

1.	Forward-port to -rcu branch dev?  This tree lives here:
	git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git

2.	Given that oldmask is used only to test to see if a new bit
	was set, why not just replace oldmask with a bool variable
	that is set to "!(rnp->expmaskinitnext & mask)" before the
	bit is ORed into rnp->expmaskinitnext?

3.	Put the comment inside the "if" statement with the
	smp_store_release().

4.	In -rcu, you will find a ASSERT_EXCLUSIVE_WRITER() statement
	that should also be placed inside the "if" statement with
	the smp_store_release().

							Thanx, Paul

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  kernel/rcu/tree.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d91c9156fab2..f0d9251fa663 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3364,7 +3364,6 @@ void rcu_cpu_starting(unsigned int cpu)
>  {
>  	unsigned long flags;
>  	unsigned long mask;
> -	int nbits;
>  	unsigned long oldmask;
>  	struct rcu_data *rdp;
>  	struct rcu_node *rnp;
> @@ -3381,10 +3380,9 @@ void rcu_cpu_starting(unsigned int cpu)
>  	rnp->qsmaskinitnext |= mask;
>  	oldmask = rnp->expmaskinitnext;
>  	rnp->expmaskinitnext |= mask;
> -	oldmask ^= rnp->expmaskinitnext;
> -	nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
>  	/* Allow lockless access for expedited grace periods. */
> -	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
> +	if (!(oldmask & mask))
> +		smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + 1); /* ^^^ */
>  	rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>  	rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>  	rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> -- 
> 2.23.0
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcu: simplify the calculation of rcu_state.ncpus
  2020-04-18  3:19 ` Paul E. McKenney
@ 2020-04-18 22:02   ` Wei Yang
  2020-04-19 15:24     ` Paul E. McKenney
  0 siblings, 1 reply; 5+ messages in thread
From: Wei Yang @ 2020-04-18 22:02 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: josh, rcu, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 11:19 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Fri, Apr 17, 2020 at 09:39:51PM +0000, Wei Yang wrote:
> > There is only 1 bit set in mask, which means the difference between
> > oldmask and the new one would be at the position where the bit is set in
> > mask.
> >
> > Based on this knowledge, rcu_state.ncpus could be calculated by checking
> > whether mask is already set in oldmask.
>
> Nice!!!  Good eyes!
>
> > BTW, the comment at the last of this line is mysterious. Not sure it
> > could be removed or not.
>
> The "^^^" in that comment says to look at the comment on the preceding
> line.  Memory-ordering functions like smp_store_release() are supposed
> to have comments indicating what they are ordering.  ;-)
>
> Could you please do the following things and resubmit?
>
> 1.      Forward-port to -rcu branch dev?  This tree lives here:
>         git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>
> 2.      Given that oldmask is used only to test to see if a new bit
>         was set, why not just replace oldmask with a bool variable
>         that is set to "!(rnp->expmaskinitnext & mask)" before the
>         bit is ORed into rnp->expmaskinitnext?
>
> 3.      Put the comment inside the "if" statement with the
>         smp_store_release().
>
> 4.      In -rcu, you will find a ASSERT_EXCLUSIVE_WRITER() statement
>         that should also be placed inside the "if" statement with
>         the smp_store_release().
>

Oops, my email client EAT this mail. Hope this mail will not be banned.

I adjust the code a little according to your suggestion like below. Is this
what you expected?

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index f288477ee1c2..f01367a80b70 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3732,10 +3732,9 @@ void rcu_cpu_starting(unsigned int cpu)
 {
        unsigned long flags;
        unsigned long mask;
-       int nbits;
-       unsigned long oldmask;
        struct rcu_data *rdp;
        struct rcu_node *rnp;
+       bool has_seen;

        if (per_cpu(rcu_cpu_started, cpu))
                return;
@@ -3747,13 +3746,13 @@ void rcu_cpu_starting(unsigned int cpu)
        mask = rdp->grpmask;
        raw_spin_lock_irqsave_rcu_node(rnp, flags);
        WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
-       oldmask = rnp->expmaskinitnext;
+       has_seen = rnp->expmaskinitnext & mask;
        rnp->expmaskinitnext |= mask;
-       oldmask ^= rnp->expmaskinitnext;
-       nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
-       /* Allow lockless access for expedited grace periods. */
-       smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
-       ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
+       if (!has_seen) {
+               /* Allow lockless access for expedited grace periods. */
+               smp_store_release(&rcu_state.ncpus, rcu_state.ncpus +
1); /* ^^^ */
+               ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
+       }
        rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
        rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
        rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);

>                                                         Thanx, Paul
>
> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > ---
> >  kernel/rcu/tree.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index d91c9156fab2..f0d9251fa663 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3364,7 +3364,6 @@ void rcu_cpu_starting(unsigned int cpu)
> >  {
> >       unsigned long flags;
> >       unsigned long mask;
> > -     int nbits;
> >       unsigned long oldmask;
> >       struct rcu_data *rdp;
> >       struct rcu_node *rnp;
> > @@ -3381,10 +3380,9 @@ void rcu_cpu_starting(unsigned int cpu)
> >       rnp->qsmaskinitnext |= mask;
> >       oldmask = rnp->expmaskinitnext;
> >       rnp->expmaskinitnext |= mask;
> > -     oldmask ^= rnp->expmaskinitnext;
> > -     nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
> >       /* Allow lockless access for expedited grace periods. */
> > -     smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
> > +     if (!(oldmask & mask))
> > +             smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + 1); /* ^^^ */
> >       rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> >       rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> >       rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> > --
> > 2.23.0
> >

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcu: simplify the calculation of rcu_state.ncpus
  2020-04-18 22:02   ` Wei Yang
@ 2020-04-19 15:24     ` Paul E. McKenney
  2020-04-19 21:48       ` Wei Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Paul E. McKenney @ 2020-04-19 15:24 UTC (permalink / raw)
  To: Wei Yang; +Cc: josh, rcu, Linux Kernel Mailing List

On Sun, Apr 19, 2020 at 06:02:12AM +0800, Wei Yang wrote:
> On Sat, Apr 18, 2020 at 11:19 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Fri, Apr 17, 2020 at 09:39:51PM +0000, Wei Yang wrote:
> > > There is only 1 bit set in mask, which means the difference between
> > > oldmask and the new one would be at the position where the bit is set in
> > > mask.
> > >
> > > Based on this knowledge, rcu_state.ncpus could be calculated by checking
> > > whether mask is already set in oldmask.
> >
> > Nice!!!  Good eyes!
> >
> > > BTW, the comment at the last of this line is mysterious. Not sure it
> > > could be removed or not.
> >
> > The "^^^" in that comment says to look at the comment on the preceding
> > line.  Memory-ordering functions like smp_store_release() are supposed
> > to have comments indicating what they are ordering.  ;-)
> >
> > Could you please do the following things and resubmit?
> >
> > 1.      Forward-port to -rcu branch dev?  This tree lives here:
> >         git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
> >
> > 2.      Given that oldmask is used only to test to see if a new bit
> >         was set, why not just replace oldmask with a bool variable
> >         that is set to "!(rnp->expmaskinitnext & mask)" before the
> >         bit is ORed into rnp->expmaskinitnext?
> >
> > 3.      Put the comment inside the "if" statement with the
> >         smp_store_release().
> >
> > 4.      In -rcu, you will find a ASSERT_EXCLUSIVE_WRITER() statement
> >         that should also be placed inside the "if" statement with
> >         the smp_store_release().
> >
> 
> Oops, my email client EAT this mail. Hope this mail will not be banned.

This one made it.  ;-)

> I adjust the code a little according to your suggestion like below. Is this
> what you expected?

Much better, thank you!

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index f288477ee1c2..f01367a80b70 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3732,10 +3732,9 @@ void rcu_cpu_starting(unsigned int cpu)
>  {
>         unsigned long flags;
>         unsigned long mask;
> -       int nbits;
> -       unsigned long oldmask;
>         struct rcu_data *rdp;
>         struct rcu_node *rnp;
> +       bool has_seen;

Could you please use a more descriptive name, perhaps something like
"newcpu"?  Yes, "has_seen" does make sense, but it leaves the reader
the question "has seen what?".

>         if (per_cpu(rcu_cpu_started, cpu))
>                 return;
> @@ -3747,13 +3746,13 @@ void rcu_cpu_starting(unsigned int cpu)
>         mask = rdp->grpmask;
>         raw_spin_lock_irqsave_rcu_node(rnp, flags);
>         WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
> -       oldmask = rnp->expmaskinitnext;
> +       has_seen = rnp->expmaskinitnext & mask;
>         rnp->expmaskinitnext |= mask;
> -       oldmask ^= rnp->expmaskinitnext;
> -       nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
> -       /* Allow lockless access for expedited grace periods. */
> -       smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
> -       ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
> +       if (!has_seen) {
> +               /* Allow lockless access for expedited grace periods. */
> +               smp_store_release(&rcu_state.ncpus, rcu_state.ncpus +
> 1); /* ^^^ */
> +               ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);

One advantage of flipping the conditional is that it allows dropping the
"if" statement, which saves a few lines of code and gets closer to your
original suggestion:

	newcpu = !(rnp->expmaskinitnext & mask);
	rnp->expmaskinitnext |= mask;
	/* Allow lockless access for expedited grace periods. */
	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + newcpu); /* ^^^ */
	ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);

Yes, this does execute a store-release that doesn't change anything,
but this is OK because (1) CPU hotplug is infrequent and (2) CPU hotplug
has very high overhead.  The overhead of the store is thus neglible.
(But please do not try this to heavily contended cachelines on fastpaths!

And yes, I should have thought of this in response to your initial patch,
but then again, I should have thought of this when writing this code in
the first place, shouldn't I have?  ;-)

							Thanx, Paul

> +       }
>         rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>         rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>         rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> 
> >                                                         Thanx, Paul
> >
> > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > > ---
> > >  kernel/rcu/tree.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index d91c9156fab2..f0d9251fa663 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3364,7 +3364,6 @@ void rcu_cpu_starting(unsigned int cpu)
> > >  {
> > >       unsigned long flags;
> > >       unsigned long mask;
> > > -     int nbits;
> > >       unsigned long oldmask;
> > >       struct rcu_data *rdp;
> > >       struct rcu_node *rnp;
> > > @@ -3381,10 +3380,9 @@ void rcu_cpu_starting(unsigned int cpu)
> > >       rnp->qsmaskinitnext |= mask;
> > >       oldmask = rnp->expmaskinitnext;
> > >       rnp->expmaskinitnext |= mask;
> > > -     oldmask ^= rnp->expmaskinitnext;
> > > -     nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
> > >       /* Allow lockless access for expedited grace periods. */
> > > -     smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
> > > +     if (!(oldmask & mask))
> > > +             smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + 1); /* ^^^ */
> > >       rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
> > >       rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
> > >       rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> > > --
> > > 2.23.0
> > >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcu: simplify the calculation of rcu_state.ncpus
  2020-04-19 15:24     ` Paul E. McKenney
@ 2020-04-19 21:48       ` Wei Yang
  0 siblings, 0 replies; 5+ messages in thread
From: Wei Yang @ 2020-04-19 21:48 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: Wei Yang, josh, rcu, Linux Kernel Mailing List

On Sun, Apr 19, 2020 at 08:24:45AM -0700, Paul E. McKenney wrote:
>On Sun, Apr 19, 2020 at 06:02:12AM +0800, Wei Yang wrote:
>> On Sat, Apr 18, 2020 at 11:19 AM Paul E. McKenney <paulmck@kernel.org> wrote:
>> >
>> > On Fri, Apr 17, 2020 at 09:39:51PM +0000, Wei Yang wrote:
>> > > There is only 1 bit set in mask, which means the difference between
>> > > oldmask and the new one would be at the position where the bit is set in
>> > > mask.
>> > >
>> > > Based on this knowledge, rcu_state.ncpus could be calculated by checking
>> > > whether mask is already set in oldmask.
>> >
>> > Nice!!!  Good eyes!
>> >
>> > > BTW, the comment at the last of this line is mysterious. Not sure it
>> > > could be removed or not.
>> >
>> > The "^^^" in that comment says to look at the comment on the preceding
>> > line.  Memory-ordering functions like smp_store_release() are supposed
>> > to have comments indicating what they are ordering.  ;-)
>> >
>> > Could you please do the following things and resubmit?
>> >
>> > 1.      Forward-port to -rcu branch dev?  This tree lives here:
>> >         git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>> >
>> > 2.      Given that oldmask is used only to test to see if a new bit
>> >         was set, why not just replace oldmask with a bool variable
>> >         that is set to "!(rnp->expmaskinitnext & mask)" before the
>> >         bit is ORed into rnp->expmaskinitnext?
>> >
>> > 3.      Put the comment inside the "if" statement with the
>> >         smp_store_release().
>> >
>> > 4.      In -rcu, you will find a ASSERT_EXCLUSIVE_WRITER() statement
>> >         that should also be placed inside the "if" statement with
>> >         the smp_store_release().
>> >
>> 
>> Oops, my email client EAT this mail. Hope this mail will not be banned.
>
>This one made it.  ;-)
>
>> I adjust the code a little according to your suggestion like below. Is this
>> what you expected?
>
>Much better, thank you!
>
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index f288477ee1c2..f01367a80b70 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -3732,10 +3732,9 @@ void rcu_cpu_starting(unsigned int cpu)
>>  {
>>         unsigned long flags;
>>         unsigned long mask;
>> -       int nbits;
>> -       unsigned long oldmask;
>>         struct rcu_data *rdp;
>>         struct rcu_node *rnp;
>> +       bool has_seen;
>
>Could you please use a more descriptive name, perhaps something like
>"newcpu"?  Yes, "has_seen" does make sense, but it leaves the reader
>the question "has seen what?".
>
>>         if (per_cpu(rcu_cpu_started, cpu))
>>                 return;
>> @@ -3747,13 +3746,13 @@ void rcu_cpu_starting(unsigned int cpu)
>>         mask = rdp->grpmask;
>>         raw_spin_lock_irqsave_rcu_node(rnp, flags);
>>         WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
>> -       oldmask = rnp->expmaskinitnext;
>> +       has_seen = rnp->expmaskinitnext & mask;
>>         rnp->expmaskinitnext |= mask;
>> -       oldmask ^= rnp->expmaskinitnext;
>> -       nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
>> -       /* Allow lockless access for expedited grace periods. */
>> -       smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
>> -       ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
>> +       if (!has_seen) {
>> +               /* Allow lockless access for expedited grace periods. */
>> +               smp_store_release(&rcu_state.ncpus, rcu_state.ncpus +
>> 1); /* ^^^ */
>> +               ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
>
>One advantage of flipping the conditional is that it allows dropping the
>"if" statement, which saves a few lines of code and gets closer to your
>original suggestion:
>
>	newcpu = !(rnp->expmaskinitnext & mask);
>	rnp->expmaskinitnext |= mask;
>	/* Allow lockless access for expedited grace periods. */
>	smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + newcpu); /* ^^^ */
>	ASSERT_EXCLUSIVE_WRITER(rcu_state.ncpus);
>
>Yes, this does execute a store-release that doesn't change anything,
>but this is OK because (1) CPU hotplug is infrequent and (2) CPU hotplug
>has very high overhead.  The overhead of the store is thus neglible.
>(But please do not try this to heavily contended cachelines on fastpaths!

I thought I got your point.

>
>And yes, I should have thought of this in response to your initial patch,
>but then again, I should have thought of this when writing this code in
>the first place, shouldn't I have?  ;-)
>

Faire enough.

>							Thanx, Paul
>
>> +       }
>>         rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>>         rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>>         rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
>> 
>> >                                                         Thanx, Paul
>> >
>> > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > > ---
>> > >  kernel/rcu/tree.c | 6 ++----
>> > >  1 file changed, 2 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> > > index d91c9156fab2..f0d9251fa663 100644
>> > > --- a/kernel/rcu/tree.c
>> > > +++ b/kernel/rcu/tree.c
>> > > @@ -3364,7 +3364,6 @@ void rcu_cpu_starting(unsigned int cpu)
>> > >  {
>> > >       unsigned long flags;
>> > >       unsigned long mask;
>> > > -     int nbits;
>> > >       unsigned long oldmask;
>> > >       struct rcu_data *rdp;
>> > >       struct rcu_node *rnp;
>> > > @@ -3381,10 +3380,9 @@ void rcu_cpu_starting(unsigned int cpu)
>> > >       rnp->qsmaskinitnext |= mask;
>> > >       oldmask = rnp->expmaskinitnext;
>> > >       rnp->expmaskinitnext |= mask;
>> > > -     oldmask ^= rnp->expmaskinitnext;
>> > > -     nbits = bitmap_weight(&oldmask, BITS_PER_LONG);
>> > >       /* Allow lockless access for expedited grace periods. */
>> > > -     smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + nbits); /* ^^^ */
>> > > +     if (!(oldmask & mask))
>> > > +             smp_store_release(&rcu_state.ncpus, rcu_state.ncpus + 1); /* ^^^ */
>> > >       rcu_gpnum_ovf(rnp, rdp); /* Offline-induced counter wrap? */
>> > >       rdp->rcu_onl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>> > >       rdp->rcu_onl_gp_flags = READ_ONCE(rcu_state.gp_flags);
>> > > --
>> > > 2.23.0
>> > >

-- 
Wei Yang
Help you, Help me

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-19 21:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 21:39 [PATCH] rcu: simplify the calculation of rcu_state.ncpus Wei Yang
2020-04-18  3:19 ` Paul E. McKenney
2020-04-18 22:02   ` Wei Yang
2020-04-19 15:24     ` Paul E. McKenney
2020-04-19 21:48       ` Wei Yang

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).