Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 19 Apr 2020 21:48:08 +0000
From:   Wei Yang <richard.weiyang@gmail.com>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     Wei Yang <richard.weiyang@gmail.com>, josh@joshtriplett.org,
        rcu@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] rcu: simplify the calculation of rcu_state.ncpus
Message-ID: <20200419214808.ga3fdpl22fzukawh@master>
Reply-To: Wei Yang <richard.weiyang@gmail.com>
References: <20200417213951.29837-1-richard.weiyang@gmail.com>
 <20200418031922.GR17661@paulmck-ThinkPad-P72>
 <CADZGycYVL9j7jbE1ZY_ZOv2XXv9XwWGPykoE63nJ79VrXwwUCg@mail.gmail.com>
 <20200419152445.GU17661@paulmck-ThinkPad-P72>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200419152445.GU17661@paulmck-ThinkPad-P72>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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