Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>

Subject: x86/tbl/trace: ---> .../tlb/...

- Sedat -

> When taking a CPU down for suspend and resume, a tracepoint may be called
> when the CPU has been designated offline. As tracepoints require RCU for
> protection, they must not be called if the current CPU is offline.
>
> Unfortunately, trace_tlb_flush() is called in this scenario as was noted
> by LOCKDEP:
>
> ...
>
>  Disabling non-boot CPUs ...
>  intel_pstate CPU 1 exiting
>
>  ===============================
>  smpboot: CPU 1 didn't die...
>  [ INFO: suspicious RCU usage. ]
>  3.19.0-rc7-next-20150204.1-iniza-small #1 Not tainted
>  -------------------------------
>  include/trace/events/tlb.h:35 suspicious rcu_dereference_check() usage!
>
>  other info that might help us debug this:
>
>  RCU used illegally from offline CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  no locks held by swapper/1/0.
>
>  stack backtrace:
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0-rc7-next-20150204.1-iniza-small #1
>  Hardware name: SAMSUNG ELECTRONICS CO., LTD. 530U3BI/530U4BI/530U4BH/530U3BI/530U4BI/530U4BH, BIOS 13XK 03/28/2013
>   0000000000000001 ffff88011a44fe18 ffffffff817e370d 0000000000000011
>   ffff88011a448290 ffff88011a44fe48 ffffffff810d6847 ffff8800c66b9600
>   0000000000000001 ffff88011a44c000 ffffffff81cb3900 ffff88011a44fe78
>  Call Trace:
>   [<ffffffff817e370d>] dump_stack+0x4c/0x65
>   [<ffffffff810d6847>] lockdep_rcu_suspicious+0xe7/0x120
>   [<ffffffff810b71a5>] idle_task_exit+0x205/0x2c0
>   [<ffffffff81054c4e>] play_dead_common+0xe/0x50
>   [<ffffffff81054ca5>] native_play_dead+0x15/0x140
>   [<ffffffff8102963f>] arch_cpu_idle_dead+0xf/0x20
>   [<ffffffff810cd89e>] cpu_startup_entry+0x37e/0x580
>   [<ffffffff81053e20>] start_secondary+0x140/0x150
>  intel_pstate CPU 2 exiting
>
> ...
>
> By converting the tlb_flush tracepoint to a TRACE_EVENT_CONDITION where the
> condition is cpu_online(smp_processor_id()), we can avoid calling RCU protected
> code when the CPU is offline.
>
> Link: http://lkml.kernel.org/r/CA+icZUUGiGDoL5NU8RuxKzFjoLjEKRtUWx=JB8B9a0EQv-eGzQ@mail.gmail.com
>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/trace/events/tlb.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/trace/events/tlb.h b/include/trace/events/tlb.h
> index 13391d288107..0e7635765153 100644
> --- a/include/trace/events/tlb.h
> +++ b/include/trace/events/tlb.h
> @@ -13,11 +13,13 @@
>         { TLB_LOCAL_SHOOTDOWN,          "local shootdown" },            \
>         { TLB_LOCAL_MM_SHOOTDOWN,       "local mm shootdown" }
>
> -TRACE_EVENT(tlb_flush,
> +TRACE_EVENT_CONDITION(tlb_flush,
>
>         TP_PROTO(int reason, unsigned long pages),
>         TP_ARGS(reason, pages),
>
> +       TP_CONDITION(cpu_online(smp_processor_id())),
> +
>         TP_STRUCT__entry(
>                 __field(          int, reason)
>                 __field(unsigned long,  pages)
> --
> 2.1.4
>
>

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Fri, 6 Feb 2015 21:11:57 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:

> On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >
> 
> Subject: x86/tbl/trace: ---> .../tlb/...
> 

But I wanted "To Be Loved".

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Fri, Feb 6, 2015 at 9:21 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 6 Feb 2015 21:11:57 +0100
> Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
>> On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>> >
>>
>> Subject: x86/tbl/trace: ---> .../tlb/...
>>
>
> But I wanted "To Be Loved".
>

Sorry, the patch is not "applicable" :-)

- Sedat -

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Fri, 6 Feb 2015 21:23:13 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:

> On Fri, Feb 6, 2015 at 9:21 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Fri, 6 Feb 2015 21:11:57 +0100
> > Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> >> On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >> >
> >>
> >> Subject: x86/tbl/trace: ---> .../tlb/...
> >>
> >
> > But I wanted "To Be Loved".
> >
> 
> Sorry, the patch is not "applicable" :-)

How about, I wanted "x86 to be loved by trace"? Or vice versa.

Applicable then?

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Fri, 6 Feb 2015 21:11:57 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:

> On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> >
> 
> Subject: x86/tbl/trace: ---> .../tlb/...
> 

But you missed that the cover letter was for x85 ;-)

(I did too, it was pointed out to me on IRC).

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Fri, Feb 6, 2015 at 10:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 6 Feb 2015 21:11:57 +0100
> Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
>> On Fri, Feb 6, 2015 at 9:06 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>> >
>>
>> Subject: x86/tbl/trace: ---> .../tlb/...
>>
>
> But you missed that the cover letter was for x85 ;-)
>
> (I did too, it was pointed out to me on IRC).
>

Man man man you write and test your stuff still on ancient x85 arch.
What's coming next... revive/reinclude i386 code?

One glass of bitter lemon for you.
Coffee is sold out here.

- Sedat -

https://www.youtube.com/watch?v=5Gv-Cj2e0qs

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Fri, 6 Feb 2015 22:39:52 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:

> Man man man you write and test your stuff still on ancient x85 arch.
> What's coming next... revive/reinclude i386 code?

Real mode for Real men! Yeah Baby!

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Fri, Feb 6, 2015 at 10:42 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 6 Feb 2015 22:39:52 +0100
> Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
>> Man man man you write and test your stuff still on ancient x85 arch.
>> What's coming next... revive/reinclude i386 code?
>
> Real mode for Real men! Yeah Baby!
>

I hope you enjoyed your glass of bitter lemon.

After Paul agreed that this one can also go through your tree I hope
to see an refreshed version in your linux-trace tree.

- Sedat -

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Dave Hansen]

Feel free to add to the pair:

Acked-by: Dave Hansen <dave@sr71.net>

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Sat, Feb 7, 2015 at 11:14 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Sat, Feb 07, 2015 at 04:52:05PM -0500, Steven Rostedt wrote:
>> On Sat, 7 Feb 2015 12:09:48 -0800
>> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>>
>> >    The tag sequence has the meaning of:
>> >      git cherry-pick a1f84a3
>> >      git cherry-pick 1b9508f
>> >      git cherry-pick fd21073
>> >      git cherry-pick <this commit>
>> >
>> > Does that do what you need?
>>
>> Note, for this case it really doesn't apply, because one patch does not
>> depend on the other.
>>
>> The real bug is that a tracepoint can be called when RCU is not
>> watching (cpu is offline). That bug was introduced in 3.17 and is fixed
>> by patch 2 with the conditional trace event.
>>
>> When that bug was fixed, it showed that another bug exists. That is
>> that lockdep should not complain if the conditional prevents the bad
>> RCU from happening, and this bug was introduced in 3.18. This was fixed
>> by the first patch.
>>
>> They really are two entirely separate bugs, it just happens that the
>> test case Sedat had happened to trigger both of them. This is why I
>> really don't see why the two need to reference each other.
>>
>> I'm also going to modify patch 1 to not mention porting the other
>> commit (that patch 1 fixes) to 3.17 (from 3.18), as that other commit is
>> just a debugging tool and not something that satisfies being
>> backported, and the patch that fixes it shouldn't be backported to 3.17
>> either, only to 3.18.
>
> Thank you for the explanation!  I guess I needed to have kept a scorecard
> on this one.  ;-)
>

My misunderstanding and thanks again for the clarification.
As Steve pointed out, /me needs both patches to fix my broken testcase.
My wish was not to forget affected (previous) linux-stable releases
when dealing with the issue.

- Sedat -

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Paul E. McKenney]

On Sat, Feb 07, 2015 at 04:52:05PM -0500, Steven Rostedt wrote:
> On Sat, 7 Feb 2015 12:09:48 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> >    The tag sequence has the meaning of:
> >      git cherry-pick a1f84a3
> >      git cherry-pick 1b9508f
> >      git cherry-pick fd21073
> >      git cherry-pick <this commit>
> > 
> > Does that do what you need?
> 
> Note, for this case it really doesn't apply, because one patch does not
> depend on the other.
> 
> The real bug is that a tracepoint can be called when RCU is not
> watching (cpu is offline). That bug was introduced in 3.17 and is fixed
> by patch 2 with the conditional trace event.
> 
> When that bug was fixed, it showed that another bug exists. That is
> that lockdep should not complain if the conditional prevents the bad
> RCU from happening, and this bug was introduced in 3.18. This was fixed
> by the first patch.
> 
> They really are two entirely separate bugs, it just happens that the
> test case Sedat had happened to trigger both of them. This is why I
> really don't see why the two need to reference each other.
> 
> I'm also going to modify patch 1 to not mention porting the other
> commit (that patch 1 fixes) to 3.17 (from 3.18), as that other commit is
> just a debugging tool and not something that satisfies being
> backported, and the patch that fixes it shouldn't be backported to 3.17
> either, only to 3.18.

Thank you for the explanation!  I guess I needed to have kept a scorecard
on this one.  ;-)

							Thanx, Paul

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Sat, 7 Feb 2015 12:09:48 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

>    The tag sequence has the meaning of:
>      git cherry-pick a1f84a3
>      git cherry-pick 1b9508f
>      git cherry-pick fd21073
>      git cherry-pick <this commit>
> 
> Does that do what you need?

Note, for this case it really doesn't apply, because one patch does not
depend on the other.

The real bug is that a tracepoint can be called when RCU is not
watching (cpu is offline). That bug was introduced in 3.17 and is fixed
by patch 2 with the conditional trace event.

When that bug was fixed, it showed that another bug exists. That is
that lockdep should not complain if the conditional prevents the bad
RCU from happening, and this bug was introduced in 3.18. This was fixed
by the first patch.

They really are two entirely separate bugs, it just happens that the
test case Sedat had happened to trigger both of them. This is why I
really don't see why the two need to reference each other.

I'm also going to modify patch 1 to not mention porting the other
commit (that patch 1 fixes) to 3.17 (from 3.18), as that other commit is
just a debugging tool and not something that satisfies being
backported, and the patch that fixes it shouldn't be backported to 3.17
either, only to 3.18.

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Sat, Feb 7, 2015 at 9:09 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Sat, Feb 07, 2015 at 10:20:02AM -0500, Steven Rostedt wrote:
>> On Sat, 7 Feb 2015 09:01:34 +0100
>> Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>
>>
>> > - Tested-by's
>> > - Reference of 2/2 to 1/2
>>
>> The two are together in the series and fix two different bugs. They do
>> not need to reference each other.
>>
>> > - CC: stable v3.17+/v3.18+ ? (1/2 has a hint for backporting in case
>> > of 3.17+ which is BTW EOL, not sure if there are main distros
>> > supporting linux-3.17.y)
>>
>> The first patch fixes a bug introduced in 3.18. There would be no splat
>> in 3.17 because the check didn't exist then. But the RCU reference
>> while cpu offline (second bug) was introduced in 3.17 and that is what
>> the second patch fixes.
>>
>> >
>> > BTW, do you happen to know if there is a someting like a
>> > "Requires-tag" or "Precondition-tag" (for example the Fixes-tag is
>> > very helpful)?
>>
>> We don't need more tags.
>>
>> > AFAICS people place such information into the commit-message.
>> > I have seen "References:" but this is more to point to a
>> > bug-tracking-system (BTS).
>> >
>> > EXAMPLE:
>> >
>> > commit 63b03e2d2477586440741677ecac45bcf28d7b1
>> > "mutex: Always clear owner field upon mutex_unlock()"
>> >
>> > References: https://bugs.freedesktop.org/show_bug.cgi?id=87955
>> >
>>
>> I may add that, but more appropriate would be to add the commit that
>> fixes the bug to the bug report.
>>
>> > Maybe ask godfather of SubmittingPatches greg-kh?
>>
>> Heh, I haven't read that document in years.
>
> Given my confusion about stable tags, I read stable_kernel_rules.txt
> the other day, and found this:
>
>  - If the patch requires other patches as prerequisites which can be
>    cherry-picked, then this can be specified in the following format in
>    the sign-off area:
>
>      Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
>      Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
>      Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
>      Cc: <stable@vger.kernel.org> # 3.3.x
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
>
>    The tag sequence has the meaning of:
>      git cherry-pick a1f84a3
>      git cherry-pick 1b9508f
>      git cherry-pick fd21073
>      git cherry-pick <this commit>
>
> Does that do what you need?
>

( Bah, I had a look recently into rules.txt in stable-queue.git. )
That sounds cool and is what I wanted.

Thanks, Paul!

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/stable/stable-queue.git/tree/rules.txt

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Paul E. McKenney]

On Sat, Feb 07, 2015 at 10:20:02AM -0500, Steven Rostedt wrote:
> On Sat, 7 Feb 2015 09:01:34 +0100
> Sedat Dilek <sedat.dilek@gmail.com> wrote:
> 
> 
> > - Tested-by's
> > - Reference of 2/2 to 1/2
> 
> The two are together in the series and fix two different bugs. They do
> not need to reference each other.
> 
> > - CC: stable v3.17+/v3.18+ ? (1/2 has a hint for backporting in case
> > of 3.17+ which is BTW EOL, not sure if there are main distros
> > supporting linux-3.17.y)
> 
> The first patch fixes a bug introduced in 3.18. There would be no splat
> in 3.17 because the check didn't exist then. But the RCU reference
> while cpu offline (second bug) was introduced in 3.17 and that is what
> the second patch fixes.
> 
> > 
> > BTW, do you happen to know if there is a someting like a
> > "Requires-tag" or "Precondition-tag" (for example the Fixes-tag is
> > very helpful)?
> 
> We don't need more tags.
> 
> > AFAICS people place such information into the commit-message.
> > I have seen "References:" but this is more to point to a
> > bug-tracking-system (BTS).
> > 
> > EXAMPLE:
> > 
> > commit 63b03e2d2477586440741677ecac45bcf28d7b1
> > "mutex: Always clear owner field upon mutex_unlock()"
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=87955
> > 
> 
> I may add that, but more appropriate would be to add the commit that
> fixes the bug to the bug report.
> 
> > Maybe ask godfather of SubmittingPatches greg-kh?
> 
> Heh, I haven't read that document in years.

Given my confusion about stable tags, I read stable_kernel_rules.txt
the other day, and found this:

 - If the patch requires other patches as prerequisites which can be
   cherry-picked, then this can be specified in the following format in
   the sign-off area:

     Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
     Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
     Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
     Cc: <stable@vger.kernel.org> # 3.3.x
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

   The tag sequence has the meaning of:
     git cherry-pick a1f84a3
     git cherry-pick 1b9508f
     git cherry-pick fd21073
     git cherry-pick <this commit>

Does that do what you need?

							Thanx, Paul

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Sat, Feb 7, 2015 at 4:20 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sat, 7 Feb 2015 09:01:34 +0100
> Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
>
>> - Tested-by's
>> - Reference of 2/2 to 1/2
>
> The two are together in the series and fix two different bugs. They do
> not need to reference each other.
>
>> - CC: stable v3.17+/v3.18+ ? (1/2 has a hint for backporting in case
>> of 3.17+ which is BTW EOL, not sure if there are main distros
>> supporting linux-3.17.y)
>
> The first patch fixes a bug introduced in 3.18. There would be no splat
> in 3.17 because the check didn't exist then. But the RCU reference
> while cpu offline (second bug) was introduced in 3.17 and that is what
> the second patch fixes.
>
>>
>> BTW, do you happen to know if there is a someting like a
>> "Requires-tag" or "Precondition-tag" (for example the Fixes-tag is
>> very helpful)?
>
> We don't need more tags.
>
>> AFAICS people place such information into the commit-message.
>> I have seen "References:" but this is more to point to a
>> bug-tracking-system (BTS).
>>
>> EXAMPLE:
>>
>> commit 63b03e2d2477586440741677ecac45bcf28d7b1
>> "mutex: Always clear owner field upon mutex_unlock()"
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=87955
>>
>
> I may add that, but more appropriate would be to add the commit that
> fixes the bug to the bug report.
>
>> Maybe ask godfather of SubmittingPatches greg-kh?
>>
>
> Heh, I haven't read that document in years.
>

Thanks for commenting.

Feel free to add my Tested-by to 2/2.

- Sedat -

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Sat, 7 Feb 2015 09:13:23 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:
I fell over this w/o having any experiences in using the tracing
> system within the Linux-kernel.
> Can you give a brief introduction?
> Or can describe how you made your tests?
> Did you use trace-cmd in your tests?
> 
>

Some of the tests use trace-cmd, others use perf, most just do echo and
cat into the tracing directory itself.

I've added these over the years and need to clean them up and put them
someplace public. Right now they are very specific to my machines.

There's also tests in tools/testing/selftests/ftrace. But the rules
for what can go into selftests limit what I can place in there. I will
be adding more tests there soon though.

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Sat, 7 Feb 2015 09:01:34 +0100
Sedat Dilek <sedat.dilek@gmail.com> wrote:


> - Tested-by's
> - Reference of 2/2 to 1/2

The two are together in the series and fix two different bugs. They do
not need to reference each other.

> - CC: stable v3.17+/v3.18+ ? (1/2 has a hint for backporting in case
> of 3.17+ which is BTW EOL, not sure if there are main distros
> supporting linux-3.17.y)

The first patch fixes a bug introduced in 3.18. There would be no splat
in 3.17 because the check didn't exist then. But the RCU reference
while cpu offline (second bug) was introduced in 3.17 and that is what
the second patch fixes.

> 
> BTW, do you happen to know if there is a someting like a
> "Requires-tag" or "Precondition-tag" (for example the Fixes-tag is
> very helpful)?

We don't need more tags.

> AFAICS people place such information into the commit-message.
> I have seen "References:" but this is more to point to a
> bug-tracking-system (BTS).
> 
> EXAMPLE:
> 
> commit 63b03e2d2477586440741677ecac45bcf28d7b1
> "mutex: Always clear owner field upon mutex_unlock()"
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=87955
> 

I may add that, but more appropriate would be to add the commit that
fixes the bug to the bug report.

> Maybe ask godfather of SubmittingPatches greg-kh?
> 

Heh, I haven't read that document in years.

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Sat, Feb 7, 2015 at 5:02 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 6 Feb 2015 15:27:54 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
>> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
>> > Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>>
>> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> Both patches survived all my tests, now I just need an ack by from Dave
> and/or Peter, and I'll push it off to Linus.
>

I fell over this w/o having any experiences in using the tracing
system within the Linux-kernel.
Can you give a brief introduction?
Or can describe how you made your tests?
Did you use trace-cmd in your tests?

Thanks.

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/rostedt/trace-cmd.git/

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Sedat Dilek]

On Sat, Feb 7, 2015 at 5:02 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 6 Feb 2015 15:27:54 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
>> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
>> > Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>>
>> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> Both patches survived all my tests, now I just need an ack by from Dave
> and/or Peter, and I'll push it off to Linus.
>

I took both patches from linux-trace.git/ftrace/urgent.

So, both patches were updated and especially 2/2 misses...

- Tested-by's
- Reference of 2/2 to 1/2
- CC: stable v3.17+/v3.18+ ? (1/2 has a hint for backporting in case
of 3.17+ which is BTW EOL, not sure if there are main distros
supporting linux-3.17.y)

BTW, do you happen to know if there is a someting like a
"Requires-tag" or "Precondition-tag" (for example the Fixes-tag is
very helpful)?
AFAICS people place such information into the commit-message.
I have seen "References:" but this is more to point to a
bug-tracking-system (BTS).

EXAMPLE:

commit 63b03e2d2477586440741677ecac45bcf28d7b1
"mutex: Always clear owner field upon mutex_unlock()"

References: https://bugs.freedesktop.org/show_bug.cgi?id=87955

Maybe ask godfather of SubmittingPatches greg-kh?

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/rostedt/linux-trace.git/log/?h=ftrace/urgent

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

On Fri, 6 Feb 2015 15:27:54 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> > Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Both patches survived all my tests, now I just need an ack by from Dave
and/or Peter, and I'll push it off to Linus.

-- Steve

Re: [PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Paul E. McKenney]

On Fri, Feb 06, 2015 at 03:06:55PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> When taking a CPU down for suspend and resume, a tracepoint may be called
> when the CPU has been designated offline. As tracepoints require RCU for
> protection, they must not be called if the current CPU is offline.
> 
> Unfortunately, trace_tlb_flush() is called in this scenario as was noted
> by LOCKDEP:
> 
> ...
> 
>  Disabling non-boot CPUs ...
>  intel_pstate CPU 1 exiting
> 
>  ===============================
>  smpboot: CPU 1 didn't die...
>  [ INFO: suspicious RCU usage. ]
>  3.19.0-rc7-next-20150204.1-iniza-small #1 Not tainted
>  -------------------------------
>  include/trace/events/tlb.h:35 suspicious rcu_dereference_check() usage!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from offline CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  no locks held by swapper/1/0.
> 
>  stack backtrace:
>  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0-rc7-next-20150204.1-iniza-small #1
>  Hardware name: SAMSUNG ELECTRONICS CO., LTD. 530U3BI/530U4BI/530U4BH/530U3BI/530U4BI/530U4BH, BIOS 13XK 03/28/2013
>   0000000000000001 ffff88011a44fe18 ffffffff817e370d 0000000000000011
>   ffff88011a448290 ffff88011a44fe48 ffffffff810d6847 ffff8800c66b9600
>   0000000000000001 ffff88011a44c000 ffffffff81cb3900 ffff88011a44fe78
>  Call Trace:
>   [<ffffffff817e370d>] dump_stack+0x4c/0x65
>   [<ffffffff810d6847>] lockdep_rcu_suspicious+0xe7/0x120
>   [<ffffffff810b71a5>] idle_task_exit+0x205/0x2c0
>   [<ffffffff81054c4e>] play_dead_common+0xe/0x50
>   [<ffffffff81054ca5>] native_play_dead+0x15/0x140
>   [<ffffffff8102963f>] arch_cpu_idle_dead+0xf/0x20
>   [<ffffffff810cd89e>] cpu_startup_entry+0x37e/0x580
>   [<ffffffff81053e20>] start_secondary+0x140/0x150
>  intel_pstate CPU 2 exiting
> 
> ...
> 
> By converting the tlb_flush tracepoint to a TRACE_EVENT_CONDITION where the
> condition is cpu_online(smp_processor_id()), we can avoid calling RCU protected
> code when the CPU is offline.
> 
> Link: http://lkml.kernel.org/r/CA+icZUUGiGDoL5NU8RuxKzFjoLjEKRtUWx=JB8B9a0EQv-eGzQ@mail.gmail.com
> 
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  include/trace/events/tlb.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/tlb.h b/include/trace/events/tlb.h
> index 13391d288107..0e7635765153 100644
> --- a/include/trace/events/tlb.h
> +++ b/include/trace/events/tlb.h
> @@ -13,11 +13,13 @@
>  	{ TLB_LOCAL_SHOOTDOWN,		"local shootdown" },		\
>  	{ TLB_LOCAL_MM_SHOOTDOWN,	"local mm shootdown" }
> 
> -TRACE_EVENT(tlb_flush,
> +TRACE_EVENT_CONDITION(tlb_flush,
> 
>  	TP_PROTO(int reason, unsigned long pages),
>  	TP_ARGS(reason, pages),
> 
> +	TP_CONDITION(cpu_online(smp_processor_id())),
> +
>  	TP_STRUCT__entry(
>  		__field(	  int, reason)
>  		__field(unsigned long,  pages)
> -- 
> 2.1.4
> 
> 

[PATCH 2/2] x86/tbl/trace: Do not trace on CPU that is offline

[Steven Rostedt]

[-- Attachment #1: 0002-x86-tbl-trace-Do-not-trace-on-CPU-that-is-offline.patch --]
[-- Type: text/plain, Size: 2828 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

When taking a CPU down for suspend and resume, a tracepoint may be called
when the CPU has been designated offline. As tracepoints require RCU for
protection, they must not be called if the current CPU is offline.

Unfortunately, trace_tlb_flush() is called in this scenario as was noted
by LOCKDEP:

...

 Disabling non-boot CPUs ...
 intel_pstate CPU 1 exiting

 ===============================
 smpboot: CPU 1 didn't die...
 [ INFO: suspicious RCU usage. ]
 3.19.0-rc7-next-20150204.1-iniza-small #1 Not tainted
 -------------------------------
 include/trace/events/tlb.h:35 suspicious rcu_dereference_check() usage!

 other info that might help us debug this:

 RCU used illegally from offline CPU!
 rcu_scheduler_active = 1, debug_locks = 0
 no locks held by swapper/1/0.

 stack backtrace:
 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0-rc7-next-20150204.1-iniza-small #1
 Hardware name: SAMSUNG ELECTRONICS CO., LTD. 530U3BI/530U4BI/530U4BH/530U3BI/530U4BI/530U4BH, BIOS 13XK 03/28/2013
  0000000000000001 ffff88011a44fe18 ffffffff817e370d 0000000000000011
  ffff88011a448290 ffff88011a44fe48 ffffffff810d6847 ffff8800c66b9600
  0000000000000001 ffff88011a44c000 ffffffff81cb3900 ffff88011a44fe78
 Call Trace:
  [<ffffffff817e370d>] dump_stack+0x4c/0x65
  [<ffffffff810d6847>] lockdep_rcu_suspicious+0xe7/0x120
  [<ffffffff810b71a5>] idle_task_exit+0x205/0x2c0
  [<ffffffff81054c4e>] play_dead_common+0xe/0x50
  [<ffffffff81054ca5>] native_play_dead+0x15/0x140
  [<ffffffff8102963f>] arch_cpu_idle_dead+0xf/0x20
  [<ffffffff810cd89e>] cpu_startup_entry+0x37e/0x580
  [<ffffffff81053e20>] start_secondary+0x140/0x150
 intel_pstate CPU 2 exiting

...

By converting the tlb_flush tracepoint to a TRACE_EVENT_CONDITION where the
condition is cpu_online(smp_processor_id()), we can avoid calling RCU protected
code when the CPU is offline.

Link: http://lkml.kernel.org/r/CA+icZUUGiGDoL5NU8RuxKzFjoLjEKRtUWx=JB8B9a0EQv-eGzQ@mail.gmail.com

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/events/tlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/tlb.h b/include/trace/events/tlb.h
index 13391d288107..0e7635765153 100644
--- a/include/trace/events/tlb.h
+++ b/include/trace/events/tlb.h
@@ -13,11 +13,13 @@
 	{ TLB_LOCAL_SHOOTDOWN,		"local shootdown" },		\
 	{ TLB_LOCAL_MM_SHOOTDOWN,	"local mm shootdown" }
 
-TRACE_EVENT(tlb_flush,
+TRACE_EVENT_CONDITION(tlb_flush,
 
 	TP_PROTO(int reason, unsigned long pages),
 	TP_ARGS(reason, pages),
 
+	TP_CONDITION(cpu_online(smp_processor_id())),
+
 	TP_STRUCT__entry(
 		__field(	  int, reason)
 		__field(unsigned long,  pages)
-- 
2.1.4