lkml.org 
[lkml]   [2012]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] tracing: Track preempt disabling

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Ingo,
>
> I've been maintaining your PREEMPT_TRACE patch from the old
> latency tracer for a long time. It some how was dropped from
> the -rt patchset that Thomas maintains. Recently it came in
> very handy debugging some of the hotplug issues that I was
> dealing with in the -rt patch, and I was thinking, why isn't
> this in mainline?
>
> I'm not the author of it, but I have modified it to keep it
> working all these years, and even updated some of the
> documentation/comments. But it is your patch, and I was
> wondering if you would please add it to your tree for 3.4
> merge window. Feel free to modify it, if there's something you
> don't like about it. Like I said, it is your patch ;-)

Hm, I have no deep objections - I just never found it useful
enough myself to mainline it.

> --- linux-trace.git.orig/include/linux/preempt.h
> +++ linux-trace.git/include/linux/preempt.h
> @@ -10,7 +10,8 @@
> #include <linux/linkage.h>
> #include <linux/list.h>
>
> -#if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER)
> +#if defined(CONFIG_DEBUG_PREEMPT) || defined(CONFIG_PREEMPT_TRACER) || \
> + defined(CONFIG_PREEMPT_TRACE)
> extern void add_preempt_count(int val);
> extern void sub_preempt_count(int val);

Such patterns are really crying out loud for a helper Kconfig
variable that all these tracing variants could set - and which
preempt tracking code could then use instead of the a || b || c
maze.

> --- linux-trace.git.orig/include/linux/sched.h
> +++ linux-trace.git/include/linux/sched.h
> @@ -1461,6 +1461,14 @@ struct task_struct {
> int softirqs_enabled;
> int softirq_context;
> #endif
> +
> +#define MAX_PREEMPT_TRACE 25
> +#define MAX_LOCK_STACK MAX_PREEMPT_TRACE
> +#ifdef CONFIG_PREEMPT_TRACE
> + unsigned long preempt_trace_eip[MAX_PREEMPT_TRACE];
> + unsigned long preempt_trace_parent_eip[MAX_PREEMPT_TRACE];
> +#endif

Now I remember another reason why I never mainlined it - the per
task preempt-off stack seemed overkill. At minimum we should add
a 'struct preempt_trace_entry' and stuff ip and parent_ip into
it. (the whole patch should do a s/eip/ip rename)

Also, it might make sense to also encapsulate the above into a
'struct preempt_trace_context' helper structure which is
zero-sized normally, and not infest sched.h with any of the
above details which really just the preempt tracing code should
care about.

> +#ifdef CONFIG_PREEMPT_TRACE
> +void print_preempt_trace(struct task_struct *tsk);
> +#else
> +static inline void print_preempt_trace(struct task_struct *tsk) { }
> +#endif

This too should be hidden in a tracing header I guess.

> #if defined(CONFIG_PREEMPT) && (defined(CONFIG_DEBUG_PREEMPT) || \
> - defined(CONFIG_PREEMPT_TRACER))
> + defined(CONFIG_PREEMPT_TRACER) || \
> + defined(CONFIG_PREEMPT_TRACE))

the new Kconfig switch could be used here.
> @@ -3030,6 +3034,15 @@ void __kprobes add_preempt_count(int val
> return;
> #endif
> preempt_count() += val;
> +#ifdef CONFIG_PREEMPT_TRACE
> + if (val <= 10) {

where does that '10' limit come from and why is it significant?
I don't remember the details anymore.

> + unsigned int idx = preempt_count() & PREEMPT_MASK;
> + if (idx < MAX_PREEMPT_TRACE) {
> + current->preempt_trace_eip[idx] = eip;
> + current->preempt_trace_parent_eip[idx] = parent_eip;
> + }

It might make sense to detach this structure from struct
task_struct and allocate it on demand?


> +config PREEMPT_TRACE
> + bool "Keep a record of preempt disabled locations"

'keep a per task record'

We already do preempt off tracing - just not on a per task
stack/history basis.

> + depends on DEBUG_KERNEL
> + depends on PREEMPT
> + select TRACING
> + help
> + When enabled, it keeps track of the last 25 locations that disabled
> + preemption. This is useful to debug a "scheduling while atomic" error.
> + Sometimes it is hard to find what left preemption disabled, and this
> + utility can help.

This is somewhat inaccurate. What we maintain is the *stack* of
preemption disabling, at most 25 levels deep.

In most common cases this will not trace back 25 locations that
disabled preemption.

So, having read this patch once again, I now have an even lower
opinion of it :-/

Firstly, it's somewhat superfluous, because in theory a perfect
preempt on/off trace recording of the whole system is an
equivalent replacement of it.

Secondly, we already have a nested execution stack: the lockdep
held-locks stack. Could we perhaps extend *that* facility and
allow it to (optionally) also print preempt off sections?

So, my opinion about it is a firm dunno.

Thanks,

Ingo


\
 
 \ /
  Last update: 2012-03-05 16:45    [W:0.039 / U:1.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site