Re: [PATCH 1/7] rcu: Fix preempt-unsafe debug check of rcu extended quiescent state

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Sep 26, 2011 at 06:04:06PM -0400, Pavel Ivanov wrote:
> On Mon, Sep 26, 2011 at 6:19 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > In the rcu_check_extended_qs() function that is used to check
> > illegal uses of RCU under extended quiescent states, we look
> > at the local value of dynticks that is even if an extended
> > quiescent state or odd otherwise.
> >
> > We are looking at it without disabling the preemption though
> > and this opens a race window where we may read the state of
> > a remote CPU instead, like in the following scenario:
> >
> > CPU 1                                                        CPU 2
> >
> > bool rcu_check_extended_qs(void)
> > {
> >        struct rcu_dynticks *rdtp;
> >
> >        rdtp = &per_cpu(rcu_dynticks,
> >                        raw_smp_processor_id());
> >
> >        < ---- Task is migrated here ---- >
> >
> >        // CPU 1 goes idle and increase rdtp->dynticks
> >                                                        /* Here we are reading the value for
> >                                                        the remote CPU 1 instead of the local CPU */
> >                                                        if (atomic_read(&rdtp->dynticks) & 0x1)
> >                                                                return false;
> >
> >                                                        return true;
> >                                                        }
> >
> > The possible result of this is false positive return value of that
> > function, suggesting we are in an extended quiescent state in random
> > places.
> 
> How is this different from what your patch allows?
> 
> CPU 1                                               CPU 2
> 
> bool rcu_check_extended_qs(void)
> {
>        struct rcu_dynticks *rdtp =
>                &get_cpu_var(rcu_dynticks);
>        bool ext_qs = true;
> 
>        if (atomic_read(&rdtp->dynticks) & 0x1)
>                ext_qs = false;
> 
>        put_cpu_var(rcu_dynticks);
> 
>        < ---- Task is migrated here ---- >
> 
>                                                   /* Here we return true/false
>                                                      based on the value for the
>                                                      remote CPU 1 instead of the
>                                                      local CPU */
> 
>                                                   return ext_qs;
>                                                   }
> 
> 
> Looks like it can result in the same false positive result.

Why?

While calling rcu_read_lock(), the task can still be migrated
for example from CPU 0 to CPU 1, until we do our check
in rcu_check_extended_qs() with preemption disabled. But that's
not a problem, we are going to do our check either in CPU 0 or
CPU 1, that's doesn't matter much.

What matters is that we do that check by ensuring we are really
checking the value of the cpu var in the CPU we are currently
running and not some other random one that can change its dynticks
value at the same time.