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
In-Reply-To: <20180810215511.60d5882c8efd271c01043881@kernel.org>
References: <20180806034049.67949-1-joel@joelfernandes.org>
 <20180806230536.ef217b2c2c008838606bedc3@kernel.org> <CAJWu+orzTpcjAcWRmgE8bLtK6BfzfQ_SW1WMho5Y-nsiACTCyQ@mail.gmail.com>
 <20180810215511.60d5882c8efd271c01043881@kernel.org>
From:   Joel Fernandes <joelaf@google.com>
Date:   Fri, 10 Aug 2018 06:01:34 -0700
Message-ID: <CAJWu+opTKi5+LFRea1oim9-pPci13kH-Af65hQ+SjKtcazu+Bw@mail.gmail.com>
Subject: Re: [PATCH ftrace/core] tracing: irqsoff: Account for additional preempt_disable
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     "Joel Fernandes (Google)" <joel@joelfernandes.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Cc: Android Kernel" <kernel-team@android.com>,
        Ingo Molnar <mingo@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Paul McKenney <paulmck@linux.vnet.ibm.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Aug 10, 2018 at 5:55 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
[..]
>>
>> >> The other way to fix this is to just use SRCU for all tracepoints.
>> >> However we can't do that because we can't use NMIs from RCU context.
>> >>
>> >> Fixes: c3bc8fd637a9 ("tracing: Centralize preemptirq tracepoints
>> >>                       and unify their usage")
>> >> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
>> >> Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
>> >> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> >> ---
>> >>  kernel/trace/trace_irqsoff.c | 26 ++++++++++++++++++++++++++
>> >>  1 file changed, 26 insertions(+)
>> >>
>> >> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
>> >> index 770cd30cda40..ffbf1505d5bc 100644
>> >> --- a/kernel/trace/trace_irqsoff.c
>> >> +++ b/kernel/trace/trace_irqsoff.c
>> >> @@ -603,14 +603,40 @@ static void irqsoff_tracer_stop(struct trace_array *tr)
>> >>   */
>> >>  static void tracer_hardirqs_on(void *none, unsigned long a0, unsigned long a1)
>> >>  {
>> >
>> > To ensure this function must not be preempted even if we increment preempt
>> > count, I think you should check irq_disabled() whole this process, put below
>> > here.
>> >
>> >         if (unlikely(!irq_disabled()))
>> >                 return;
>> >
>> > Since irq_disabled() will be checked in irq_trace() anyway, so no problem
>> > to return here when !irq_disabled().
>>
>> IRQs can never be enabled here. The trace hooks are called only after
>> disabling interrupts, or before enabling them. Right?
>>
>
> Even though, it should be verified or atleast commented on the function header.

Ok.  ftrace/core has been fixed since since so this patch is outdated
and isn't needed any more, but I agree a separate patch doc comment
would help make it clear about this fact. Both IRQ and preempt
tracepoints work this way, they fire only within their critical
section.