MIME-Version: 1.0
In-Reply-To: <20161109173808.GJ4127@linux.vnet.ibm.com>
References: <1471382376-5443-1-git-send-email-cmetcalf@mellanox.com>
 <1605b087-2b3b-77c1-01ac-084e378f5f28@mellanox.com> <20161109014045.GI4127@linux.vnet.ibm.com>
 <CALCETrWjMrdJPHSjA64rJpVCaEtZCr-ZeE08sKP1-rbvw-4+tQ@mail.gmail.com> <20161109173808.GJ4127@linux.vnet.ibm.com>
From: Andy Lutomirski <luto@amacapital.net>
Date: Wed, 9 Nov 2016 17:44:02 -0800
Message-ID: <CALCETrXs6BkR+MhRJh5k9-BWTZOQ1=nBy5ycy+1U02ei+BJRcw@mail.gmail.com>
Subject: Re: task isolation discussion at Linux Plumbers
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>,
        Gilad Ben Yossef <giladb@mellanox.com>,
        Steven Rostedt <rostedt@goodmis.org>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Rik van Riel <riel@redhat.com>, Tejun Heo <tj@kernel.org>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>, Christoph Lameter <cl@linux.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Francis Giraldeau <francis.giraldeau@gmail.com>,
        Andi Kleen <andi@firstfloor.org>, Arnd Bergmann <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2329
Lines: 72

On Wed, Nov 9, 2016 at 9:38 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:

Are you planning on changing rcu_nmi_enter()?  It would make it easier
to figure out how they interact if I could see the code.

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index dbf20b058f48..342c8ee402d6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c


>  /*
> @@ -305,17 +318,22 @@ static void rcu_dynticks_eqs_enter(void)
>  static void rcu_dynticks_eqs_exit(void)
>  {
>         struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> +       int seq;
>
>         /*
> -        * CPUs seeing atomic_inc() must see prior idle sojourns,
> +        * CPUs seeing atomic_inc_return() must see prior idle sojourns,
>          * and we also must force ordering with the next RCU read-side
>          * critical section.
>          */
> -       smp_mb__before_atomic(); /* See above. */
> -       atomic_inc(&rdtp->dynticks);
> -       smp_mb__after_atomic(); /* See above. */
> +       seq = atomic_inc_return(&rdtp->dynticks);
>         WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> -                    !(atomic_read(&rdtp->dynticks) & 0x1));
> +                    !(seq & RCU_DYNTICK_CTRL_CTR));

I think there's still a race here.  Suppose we're running this code on
cpu n and...

> +       if (seq & RCU_DYNTICK_CTRL_MASK) {
> +               rcu_eqs_special_exit();
> +               /* Prefer duplicate flushes to losing a flush. */
> +               smp_mb__before_atomic(); /* NMI safety. */

... another CPU changes the page tables and calls rcu_eqs_special_set(n) here.

That CPU expects that we will flush prior to continuing, but we won't.
Admittedly it's highly unlikely that any stale TLB entries would be
created yet, but nothing rules it out.

> +               atomic_and(~RCU_DYNTICK_CTRL_MASK, &rdtp->dynticks);
> +       }

Maybe the way to handle it is something like:

this_cpu_write(rcu_nmi_needs_eqs_special, 1);
barrier();

/* NMI here will call rcu_eqs_special_exit() regardless of the value
in dynticks */

atomic_and(...);
smp_mb__after_atomic();
rcu_eqs_special_exit();

barrier();
this_cpu_write(rcu_nmi_needs_eqs_special, 0);


Then rcu_nmi_enter() would call rcu_eqs_special_exit() if the dynticks
bit is set *or* rcu_nmi_needs_eqs_special is set.

Does that make sense?

--Andy