Messages in this thread | | | Date | Thu, 25 Feb 2016 09:15:30 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable() |
| |
On Fri, 12 Feb 2016 16:20:20 +0100 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> The preempt_disable() invokes preempt_count_add() which saves the caller > in ->preempt_disable_ip. It uses CALLER_ADDR1 which does not look for > its caller but for the parent of the caller. Which means we get the correct > caller for something like spin_lock() unless the architectures inlines > those invocations. It is always wrong for preempt_disable() or > local_bh_disable(). > > This patch makes the function get_parent_ip() which tries > CALLER_ADDR0,1,2 if the former is a locking function. > This seems to record the preempt_disable() caller properly for > preempt_disable() itself as well as for get_cpu_var() or > local_bh_disable(). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > v1…v2: drop the header split and move it to ftrace.h instead > > include/linux/ftrace.h | 12 ++++++++++++ > include/linux/sched.h | 2 -- > kernel/sched/core.c | 14 ++------------ > kernel/softirq.c | 4 ++-- > 4 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 81de7123959d..f473d9408bc3 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -711,6 +711,18 @@ static inline void __ftrace_enabled_restore(int enabled) > #define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5)) > #define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6)) > > +static inline unsigned long get_parent_ip(void)
I wonder if we should rename this to get_lock_parent_ip(). I know you are keeping the original name, but placing it into a header makes it more visible to others, and making it a less generic name is probably a good idea.
You have to modify all users anyway, to get rid of the parameter.
Want to send a v3?
-- Steve
> +{ > + unsigned long addr = CALLER_ADDR0; > + > + if (!in_lock_functions(addr)) > + return addr; > + addr = CALLER_ADDR1; > + if (!in_lock_functions(addr)) > + return addr; > + return CALLER_ADDR2; > +} > + > #ifdef CONFIG_IRQSOFF_TRACER > extern void time_hardirqs_on(unsigned long a0, unsigned long a1); > extern void time_hardirqs_off(unsigned long a0, unsigned long a1); > diff --git a/include/linux/sched.h b/include/linux/sched.h > index a10494a94cc3..f45f947500c9 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -182,8 +182,6 @@ extern void update_cpu_load_nohz(int active); > static inline void update_cpu_load_nohz(int active) { } > #endif > > -extern unsigned long get_parent_ip(unsigned long addr); > - > extern void dump_cpu_task(int cpu); > > struct seq_file; > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9503d590e5ef..12c2527f5957 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -3010,16 +3010,6 @@ u64 scheduler_tick_max_deferment(void) > } > #endif > > -notrace unsigned long get_parent_ip(unsigned long addr) > -{ > - if (in_lock_functions(addr)) { > - addr = CALLER_ADDR2; > - if (in_lock_functions(addr)) > - addr = CALLER_ADDR3; > - } > - return addr; > -} > - > #if defined(CONFIG_PREEMPT) && (defined(CONFIG_DEBUG_PREEMPT) || \ > defined(CONFIG_PREEMPT_TRACER)) > > @@ -3041,7 +3031,7 @@ void preempt_count_add(int val) > PREEMPT_MASK - 10); > #endif > if (preempt_count() == val) { > - unsigned long ip = get_parent_ip(CALLER_ADDR1); > + unsigned long ip = get_parent_ip(); > #ifdef CONFIG_DEBUG_PREEMPT > current->preempt_disable_ip = ip; > #endif > @@ -3068,7 +3058,7 @@ void preempt_count_sub(int val) > #endif > > if (preempt_count() == val) > - trace_preempt_on(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1)); > + trace_preempt_on(CALLER_ADDR0, get_parent_ip()); > __preempt_count_sub(val); > } > EXPORT_SYMBOL(preempt_count_sub); > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 479e4436f787..ec71033a87a2 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -116,9 +116,9 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) > > if (preempt_count() == cnt) { > #ifdef CONFIG_DEBUG_PREEMPT > - current->preempt_disable_ip = get_parent_ip(CALLER_ADDR1); > + current->preempt_disable_ip = get_parent_ip(); > #endif > - trace_preempt_off(CALLER_ADDR0, get_parent_ip(CALLER_ADDR1)); > + trace_preempt_off(CALLER_ADDR0, get_parent_ip()); > } > } > EXPORT_SYMBOL(__local_bh_disable_ip);
| |