Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332431 > unrolled thread

[PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-02-12 00:30 +0100
    Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for  preempt_disable() Sebastian Andrzej Siewior <sebastian@breakpoint.cc> - 2016-02-12 00:30 +0100
    Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for  preempt_disable() Steven Rostedt <rostedt@goodmis.org> - 2016-02-12 01:50 +0100
      Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for  preempt_disable() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-02-12 16:30 +0100
        Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for  preempt_disable() Steven Rostedt <rostedt@goodmis.org> - 2016-02-12 17:30 +0100

#1332431 — [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

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>
---
 include/linux/sched.h | 13 ++++++++++++-
 kernel/sched/core.c   | 14 ++------------
 kernel/softirq.c      |  4 ++--
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a10494a94cc3..cd21fd41ba2a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -59,6 +59,7 @@ struct sched_param {
 #include <linux/gfp.h>
 #include <linux/magic.h>
 #include <linux/cgroup-defs.h>
+#include <linux/ftrace_caller.h>
 
 #include <asm/processor.h>
 
@@ -182,7 +183,17 @@ 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);
+static inline unsigned long get_parent_ip(void)
+{
+	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;
+}
 
 extern void dump_cpu_task(int cpu);
 
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);
-- 
2.7.0

[toc] | [next] | [standalone]

#1332432 — Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

Before:
 preempt_test(1) kernel_init_freeable+0x1bd/0x239
 preempt_test(2) preempt_test+0x75/0x15c
 preempt_test(3) preempt_test+0xaa/0x15c
 preempt_test(4) kernel_init_freeable+0x1bd/0x239
 preempt_test(5) kernel_init_freeable+0x1bd/0x239

After:
 preempt_test(1) preempt_test+0x2f/0x15c
 preempt_test(2) preempt_test+0x75/0x15c
 preempt_test(3) preempt_test+0xaa/0x15c
 preempt_test(4) preempt_test+0xd7/0x15c
 preempt_test(5) preempt_test+0x121/0x15c

diff --git a/init/main.c b/init/main.c
index 9e64d7097f1a..da4a4b10964a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -974,6 +974,35 @@ static int __ref kernel_init(void *unused)
 	      "See Linux Documentation/init.txt for guidance.");
 }
 
+static DEFINE_PER_CPU(unsigned long, pcpu_rtest_var);
+
+static noinline void preempt_test(void)
+{
+	spinlock_t sl;
+
+	spin_lock_init(&sl);
+
+	preempt_disable();
+	pr_err("%s(1) %pF\n", __func__, current->preempt_disable_ip);
+	preempt_enable();
+
+	spin_lock(&sl);
+	pr_err("%s(2) %pF\n", __func__, current->preempt_disable_ip);
+	spin_unlock(&sl);
+
+	spin_lock_bh(&sl);
+	pr_err("%s(3) %pF\n", __func__, current->preempt_disable_ip);
+	spin_unlock_bh(&sl);
+
+	get_cpu_var(pcpu_rtest_var);
+	pr_err("%s(4) %pF\n", __func__, current->preempt_disable_ip);
+	put_cpu_var(pcpu_rtest_var);
+
+	local_bh_disable();
+	pr_err("%s(5) %pF\n", __func__, current->preempt_disable_ip);
+	local_bh_enable();
+}
+
 static noinline void __init kernel_init_freeable(void)
 {
 	/*
@@ -1006,6 +1035,7 @@ static noinline void __init kernel_init_freeable(void)
 	page_alloc_init_late();
 
 	do_basic_setup();
+	preempt_test();
 
 	/* Open the /dev/console on the rootfs, this should never fail */
 	if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)

Sebastian

[toc] | [prev] | [next] | [standalone]

#1332476 — Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

On Fri, 12 Feb 2016 08:27:01 +0800
kbuild test robot <lkp@intel.com> wrote:

> Hi Sebastian,
> 
> [auto build test ERROR on tip/sched/core]
> [also build test ERROR on v4.5-rc3 next-20160211]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 

Gotta love Wu's autobot!

> url:    https://github.com/0day-ci/linux/commits/Sebastian-Andrzej-Siewior/ftrace-move-the-CALLER_ADDRx-macros-into-its-own-header/20160212-072259
> config: s390-allyesconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=s390 
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/ftrace_caller.h:4:0,
>                     from include/linux/sched.h:62,
>                     from include/linux/kvm_host.h:15,
>                     from arch/s390/kernel/asm-offsets.c:10:
>    arch/s390/include/asm/ftrace.h: In function 'ftrace_generate_call_insn':
> >> arch/s390/include/asm/ftrace.h:77:2: error: implicit declaration of function 'is_module_addr' [-Werror=implicit-function-declaration]  
>      target = is_module_addr((void *) ip) ? ftrace_plt : FTRACE_ADDR;
>      ^
>    In file included from include/linux/mm.h:67:0,
>                     from include/linux/kvm_host.h:17,
>                     from arch/s390/kernel/asm-offsets.c:10:
>    arch/s390/include/asm/pgtable.h: At top level:
> >> arch/s390/include/asm/pgtable.h:120:19: error: static declaration of 'is_module_addr' follows non-static declaration  
>     static inline int is_module_addr(void *addr)

Looks like you need to add this? (maybe)

-- Steve

diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
index 836c56290499..16f24f92609d 100644
--- a/arch/s390/include/asm/ftrace.h
+++ b/arch/s390/include/asm/ftrace.h
@@ -12,6 +12,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <linux/module.h>
+
 #define ftrace_return_address(n) __builtin_return_address(n)
 
 void _mcount(void);

[toc] | [prev] | [next] | [standalone]

#1332805 — Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

On 02/12/2016 01:45 AM, Steven Rostedt wrote:
> Looks like you need to add this? (maybe)

no. It will then try to include other files and complain even more.
I've sent v2 and drop this split and looks simpler now.

> -- Steve

Sebastian

[toc] | [prev] | [next] | [standalone]

#1332874 — Re: [PATCH 2/2] kernel: sched: fix preempt_disable_ip recodring for preempt_disable()

On Fri, 12 Feb 2016 16:21:53 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 02/12/2016 01:45 AM, Steven Rostedt wrote:
> > Looks like you need to add this? (maybe)  
> 
> no. It will then try to include other files and complain even more.
> I've sent v2 and drop this split and looks simpler now.
> 

Hence why I said "maybe" ;-)

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web