Re: linux-next: Tree for Feb 4

[Sedat Dilek]

On Fri, Feb 6, 2015 at 12:11 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Thu, 5 Feb 2015 23:16:21 +0100
> Sedat Dilek <sedat.dilek@xxxxxxxxx> wrote:
>
>> On Thu, Feb 5, 2015 at 11:09 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>> > On Thu, 5 Feb 2015 22:45:59 +0100
>> > Sedat Dilek <sedat.dilek@xxxxxxxxx> wrote:
>> >
>> >> Steve, this was a typo it's called tlb_flush not tlb_flush*ed*:
>> >
>> > Heh, yeah, I typed that entire line in by hand. Just be lucky that was
>> > the only typo ;-)
>> >
>> >>
>> >> # cat /sys/kernel/debug/tracing/events/tlb/tlb_flush/enable
>> >> 1
>> >>
>> >> [  391.090381] intel_pstate CPU 1 exiting
>> >> [  391.104491] smpboot: CPU 1 is now offline
>> >>
>> >
>> > Now, if you disable that (echo 0 to that file), do you still get the
>> > rcu lockdep splat if you suspend and resume?
>> >
>>
>> YES, I get the call-trace again!
>>
>
> Bah! I see where the warning comes from. In include/linux/tracepoint.h
> we have:
>
> #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
>         extern struct tracepoint __tracepoint_##name;                   \
>         static inline void trace_##name(proto)                          \
>         {                                                               \
>                 if (static_key_false(&__tracepoint_##name.key))         \
>                         __DO_TRACE(&__tracepoint_##name,                \
>                                 TP_PROTO(data_proto),                   \
>                                 TP_ARGS(data_args),                     \
>                                 TP_CONDITION(cond),,);                  \
>                 if (IS_ENABLED(CONFIG_LOCKDEP)) {                       \
>                         rcu_read_lock_sched_notrace();                  \
>                         rcu_dereference_sched(__tracepoint_##name.funcs);\
>                         rcu_read_unlock_sched_notrace();                \
>                 }                                                       \
>         }                                                               \
>
> See that if (IS_ENABLED(CONFIG_LOCKDEP))?
>

I have here...

CONFIG_LOCKDEP=y

- Sedat -

> I'm recalling this. Because tracepoints require RCU, and RCU lockdep
> doesn't trigger if a tracepoint isn't enabled (because the rcu calls
> are hidden in the __DO_TRACE() behind that static_key_false), we would
> be missing lots of rcu problem tracepoints because tests were run
> without them enabled.
>
> The answer was to add this rcu check when LOCKDEP was enabled. So no,
> adding that conditional isn't going to help, because lockdep will
> trigger here, even if it were safe because of the conditional :-/.
>
> That said, let's add this (on top of the old patch):
>

Which old patch?
"tlb: Don't do trace_tlb_flush() on offline CPUs" ?

- Sedat -

> (again, not tested)
>
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> -------
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 4b75d591eb5e..401b5bfbcdbd 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -47,7 +47,12 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
>
>                 /* Re-load page tables */
>                 load_cr3(next->pgd);
> -               trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
> +               /*
> +                * Do not check rcu when tracing is not enabled. The
> +                * tracepoint has a condition to not trace if the CPU is
> +                * offline, and rcu check will complain if it is.
> +                */
> +               trace_tlb_flush_rcu_nocheck(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
>
>                 /* Stop flush ipis for the previous mm */
>                 cpumask_clear_cpu(cpu, mm_cpumask(prev));
> @@ -84,7 +89,13 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
>                          * to make sure to use no freed page tables.
>                          */
>                         load_cr3(next->pgd);
> -                       trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
> +                       /*
> +                        * Do not check rcu when tracing is not enabled. The
> +                        * tracepoint has a condition to not trace if the CPU is
> +                        * offline, and rcu check will complain if it is.
> +                        */
> +                       trace_tlb_flush_rcu_nocheck(TLB_FLUSH_ON_TASK_SWITCH,
> +                                                   TLB_FLUSH_ALL);
>                         load_LDT_nolock(&next->context);
>                 }
>         }
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index e08e21e5f601..747a05aceb60 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -179,6 +179,14 @@ extern void syscall_unregfunc(void);
>                         rcu_read_unlock_sched_notrace();                \
>                 }                                                       \
>         }                                                               \
> +       static inline void trace_##name##_rcu_nocheck(proto)            \
> +       {                                                               \
> +               if (static_key_false(&__tracepoint_##name.key))         \
> +                       __DO_TRACE(&__tracepoint_##name,                \
> +                               TP_PROTO(data_proto),                   \
> +                               TP_ARGS(data_args),                     \
> +                               TP_CONDITION(cond),,);                  \
> +       }                                                               \
>         __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),          \
>                 PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))    \
>         static inline int                                               \
> @@ -230,6 +238,8 @@ extern void syscall_unregfunc(void);
>  #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
>         static inline void trace_##name(proto)                          \
>         { }                                                             \
> +       static inline void trace_##name##_rcu_nocheck(proto)            \
> +       { }                                                             \
>         static inline void trace_##name##_rcuidle(proto)                \
>         { }                                                             \
>         static inline int                                               \
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/