Re: [PATCH 09/16] rcu: Remove redundant check for online cpu

[Paul E. McKenney]

On Wed, Jul 23, 2014 at 10:12:54AM -0400, Pranith Kumar wrote:
> On Wed, Jul 23, 2014 at 9:50 AM, Paul E. McKenney
> <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Jul 23, 2014 at 08:59:06AM -0400, Pranith Kumar wrote:
> >> On Wed, Jul 23, 2014 at 8:21 AM, Paul E. McKenney
> >> <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> >> > On Wed, Jul 23, 2014 at 01:09:46AM -0400, Pranith Kumar wrote:
> >> >> There are two checks for an online CPU if two if() conditions. This commit
> >> >> simplies this by replacing it with only one check for the online CPU.
> >> >>
> >> >> Signed-off-by: Pranith Kumar <bobby.prani@xxxxxxxxx>
> >> >
> >> > I admit that it is very early in the morning my time, but I don't see
> >> > this change as preserving the semantics in all cases.  Please recheck
> >> > your changes to the second check.
> >> >
> >> >                                                         Thanx, Paul
> >>
> >> I guess you must be thrown off by the complementary checks, the first
> >> check is for cpu_online() and second is for cpu_is_offline(). :)
> >>
> >> Previously, if a cpu is offline, the first condition is false and the
> >> second condition is true, so we return from the second if() condition.
> >> The same semantics are being preserved.
> >
> > Fair enough!
> >
> > Nevertheless, I am not seeing this as a simplification.
> 
> I am not sure what you mean here, do you mean that both the checks are
> actually required?

I mean that the current compound tests each mean something.  Pulling out
the offline test adds lines of code and obscures that meaning.   This means
that it is easier (for me, anyway) to see why the current code is correct
than it is to see why your suggested change is correct.

							Thanx, Paul

> >> --
> >> Pranith.
> >>
> >>
> >>
> >> >
> >> >> ---
> >> >>  kernel/rcu/tree.c | 9 ++++++---
> >> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >> >> index 5dcbf36..8d598a2 100644
> >> >> --- a/kernel/rcu/tree.c
> >> >> +++ b/kernel/rcu/tree.c
> >> >> @@ -2602,15 +2602,18 @@ static void __call_rcu_core(struct rcu_state *rsp, struct rcu_data *rdp,
> >> >>  {
> >> >>       bool needwake;
> >> >>
> >> >> +     if (!cpu_online(smp_processor_id()))
> >> >> +             return;
> >> >> +
> >> >>       /*
> >> >>        * If called from an extended quiescent state, invoke the RCU
> >> >>        * core in order to force a re-evaluation of RCU's idleness.
> >> >>        */
> >> >> -     if (!rcu_is_watching() && cpu_online(smp_processor_id()))
> >> >> +     if (!rcu_is_watching())
> >> >>               invoke_rcu_core();
> >> >>
> >> >> -     /* If interrupts were disabled or CPU offline, don't invoke RCU core. */
> >> >> -     if (irqs_disabled_flags(flags) || cpu_is_offline(smp_processor_id()))
> >> >> +     /* If interrupts were disabled, don't invoke RCU core. */
> >> >> +     if (irqs_disabled_flags(flags))
> >> >>               return;
> >> >>
> >> >>       /*
> >> >> --
> >> >> 2.0.0.rc2
> >> >>
> >> >
> >>
> >>
> >>
> >> --
> >> Pranith
> >>
> >
> 
> 
> 
> -- 
> Pranith
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/