From: Eiichi Tsukata <devel@etsukata.com>
To: Peter Zijlstra <peterz@infradead.org>, Andy Lutomirski <luto@kernel.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Thomas Gleixner <tglx@linutronix.de>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Frederic Weisbecker <fweisbec@gmail.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tracing: Prevent RCU EQS breakage in preemptirq events
Date: Tue, 30 Jul 2019 10:50:36 +0900 [thread overview]
Message-ID: <d8384113-a5a7-4370-e7fb-a6c4b88325e1@etsukata.com> (raw)
In-Reply-To: <20190729102948.GY31381@hirez.programming.kicks-ass.net>
Thanks for comments.
On 2019/07/29 19:29, Peter Zijlstra wrote:
> On Sun, Jul 28, 2019 at 09:25:58PM -0700, Andy Lutomirski wrote:
>> On Sun, Jul 28, 2019 at 6:08 PM Eiichi Tsukata <devel@etsukata.com> wrote:
>
>>> 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.
>
> I think they already (try to) do that; see 'tracing_irq_cpu'.
>
Or you mean something like this?
As for trace_hardirqs_off_caller:
diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
index 4d8e99fdbbbe..d39478bcf0f2 100644
--- a/kernel/trace/trace_preemptirq.c
+++ b/kernel/trace/trace_preemptirq.c
@@ -66,7 +66,7 @@ __visible void trace_hardirqs_off_caller(unsigned long caller_addr)
if (!this_cpu_read(tracing_irq_cpu)) {
this_cpu_write(tracing_irq_cpu, 1);
tracer_hardirqs_off(CALLER_ADDR0, caller_addr);
- if (!in_nmi())
+ if (!in_nmi() && !irqs_disabled())
trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr);
}
Or
diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
index 4d8e99fdbbbe..e08c5c6ff2b3 100644
--- a/kernel/trace/trace_preemptirq.c
+++ b/kernel/trace/trace_preemptirq.c
@@ -66,8 +66,6 @@ __visible void trace_hardirqs_off_caller(unsigned long caller_addr)
if (!this_cpu_read(tracing_irq_cpu)) {
this_cpu_write(tracing_irq_cpu, 1);
tracer_hardirqs_off(CALLER_ADDR0, caller_addr);
- if (!in_nmi())
- trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr);
}
As for trace_hardirqs_on_caller, it is called when IRQs off and CONTEXT_USER.
So even though we skipped the trace event if the previous state was already IRQs on,
we will fall into the same situation.
next prev parent reply other threads:[~2019-07-30 1:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-29 1:07 [PATCH] tracing: Prevent RCU EQS breakage in preemptirq events Eiichi Tsukata
2019-07-29 4:25 ` Andy Lutomirski
2019-07-29 10:29 ` Peter Zijlstra
2019-07-30 1:50 ` Eiichi Tsukata [this message]
2019-07-30 1:59 ` Steven Rostedt
2019-07-29 15:21 ` Steven Rostedt
2019-07-30 2:00 ` Eiichi Tsukata
2019-07-30 2:15 ` Steven Rostedt
2019-07-30 4:19 ` Joel Fernandes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d8384113-a5a7-4370-e7fb-a6c4b88325e1@etsukata.com \
--to=devel@etsukata.com \
--cc=fweisbec@gmail.com \
--cc=joel@joelfernandes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).