Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190729010734.3352-1-devel@etsukata.com>
In-Reply-To: <20190729010734.3352-1-devel@etsukata.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Sun, 28 Jul 2019 21:25:58 -0700
Message-ID: <CALCETrVavLdQ8Rp+6fmTd7kJJwvRKdaEnudaiMAu8g9ZXuNfWA@mail.gmail.com>
Subject: Re: [PATCH] tracing: Prevent RCU EQS breakage in preemptirq events
To:     Eiichi Tsukata <devel@etsukata.com>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jul 28, 2019 at 6:08 PM Eiichi Tsukata <devel@etsukata.com> wrote:
>
> If context tracking is enabled, causing page fault in preemptirq
> irq_enable or irq_disable events triggers the following RCU EQS warning.
>

Yuck.

> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index be01a4d627c9..860eaf9780e5 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -148,6 +148,11 @@ void __context_tracking_exit(enum ctx_state state)
>                 return;
>
>         if (__this_cpu_read(context_tracking.state) == state) {
> +               /*
> +                * Change state before executing codes which can trigger
> +                * page fault leading unnecessary re-entrance.
> +                */
> +               __this_cpu_write(context_tracking.state, CONTEXT_KERNEL);

Seems reasonable.

>                 if (__this_cpu_read(context_tracking.active)) {
>                         /*
>                          * We are going to run code that may use RCU. Inform
> @@ -159,7 +164,6 @@ void __context_tracking_exit(enum ctx_state state)
>                                 trace_user_exit(0);
>                         }
>                 }
> -               __this_cpu_write(context_tracking.state, CONTEXT_KERNEL);
>         }
>         context_tracking_recursion_exit();
>  }
> diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
> index 4d8e99fdbbbe..031b51cb94d0 100644
> --- a/kernel/trace/trace_preemptirq.c
> +++ b/kernel/trace/trace_preemptirq.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/ftrace.h>
>  #include <linux/kprobes.h>
> +#include <linux/context_tracking.h>
>  #include "trace.h"
>
>  #define CREATE_TRACE_POINTS
> @@ -49,9 +50,14 @@ NOKPROBE_SYMBOL(trace_hardirqs_off);
>
>  __visible void trace_hardirqs_on_caller(unsigned long caller_addr)
>  {
> +       enum ctx_state prev_state;
> +
>         if (this_cpu_read(tracing_irq_cpu)) {
> -               if (!in_nmi())
> +               if (!in_nmi()) {
> +                       prev_state = exception_enter();
>                         trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr);
> +                       exception_exit(prev_state);
> +               }
>                 tracer_hardirqs_on(CALLER_ADDR0, caller_addr);
>                 this_cpu_write(tracing_irq_cpu, 0);
>         }

This seems a bit distressing.  Now we're going to do a whole bunch of
context tracking transitions for each kernel entry.  Would a better
fix me to change trace_hardirqs_on_caller to skip the trace event if
the previous state was already IRQs on and, more importantly, to skip
tracing IRQs off if IRQs were already off?  The x86 code is very
careful to avoid ever having IRQs on and CONTEXT_USER at the same
time.

--Andy