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-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
