[PATCH] percpu: use raw_local_irq_* in _this_cpu op

[PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Ming Lei]

It doesn't make sense to trace irq off or do irq flags
lock proving inside 'this_cpu' operations, so replace local_irq_*
with raw_local_irq_* in 'this_cpu' op.

Also the patch fixes one lockdep warning[1], which is caused
by the added local_irq_save/restore(flags) in this_cpu_inc
called by __debug_atomic_inc: kernel/lockdep.c

[1],
[    0.162841] ------------[ cut here ]------------
[    0.167694] WARNING: at kernel/lockdep.c:3493 check_flags+0xc0/0x1d0()
[    0.174468] Modules linked in:
[    0.177703] Backtrace:
[    0.180328] [<c00171f0>] (dump_backtrace+0x0/0x110) from [<c0412320>] (dump_stack+0x18/0x1c)
[    0.189086]  r6:c051f778 r5:00000da5 r4:00000000 r3:60000093
[    0.195007] [<c0412308>] (dump_stack+0x0/0x1c) from [<c00410e8>] (warn_slowpath_common+0x54/0x6c)
[    0.204223] [<c0041094>] (warn_slowpath_common+0x0/0x6c) from [<c0041124>] (warn_slowpath_null+0x24/0x2c)
[    0.214111]  r8:00000000 r7:00000000 r6:ee069598 r5:60000013 r4:ee082000
[    0.220825] r3:00000009
[    0.223693] [<c0041100>] (warn_slowpath_null+0x0/0x2c) from [<c0088f38>] (check_flags+0xc0/0x1d0)
[    0.232910] [<c0088e78>] (check_flags+0x0/0x1d0) from [<c008d348>] (lock_acquire+0x4c/0x11c)
[    0.241668] [<c008d2fc>] (lock_acquire+0x0/0x11c) from [<c0415aa4>] (_raw_spin_lock+0x3c/0x74)
[    0.250610] [<c0415a68>] (_raw_spin_lock+0x0/0x74) from [<c010a844>] (set_task_comm+0x20/0xc0)
[    0.259521]  r6:ee069588 r5:ee0691c0 r4:ee082000
[    0.264404] [<c010a824>] (set_task_comm+0x0/0xc0) from [<c0060780>] (kthreadd+0x28/0x108)
[    0.272857]  r8:00000000 r7:00000013 r6:c0044a08 r5:ee0691c0 r4:ee082000
[    0.279571] r3:ee083fe0
[    0.282470] [<c0060758>] (kthreadd+0x0/0x108) from [<c0044a08>] (do_exit+0x0/0x6dc)
[    0.290405]  r5:c0060758 r4:00000000
[    0.294189] ---[ end trace 1b75b31a2719ed1c ]---
[    0.299041] possible reason: unannotated irqs-on.
[    0.303955] irq event stamp: 5
[    0.307159] hardirqs last  enabled at (4): [<c001331c>] no_work_pending+0x8/0x2c
[    0.314880] hardirqs last disabled at (5): [<c0089b08>] trace_hardirqs_on_caller+0x60/0x26c
[    0.323547] softirqs last  enabled at (0): [<c003f754>] copy_process+0x33c/0xef4
[    0.331207] softirqs last disabled at (0): [<  (null)>]   (null)
[    0.337585] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/linux/percpu.h |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index 6e68d05..5ed1e38 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -294,9 +294,9 @@ do {									\
 #define _this_cpu_generic_to_op(pcp, val, op)				\
 do {									\
 	unsigned long flags;						\
-	local_irq_save(flags);						\
+	raw_local_irq_save(flags);						\
 	*__this_cpu_ptr(&(pcp)) op val;					\
-	local_irq_restore(flags);					\
+	raw_local_irq_restore(flags);					\
 } while (0)
 
 #ifndef this_cpu_write
@@ -395,10 +395,10 @@ do {									\
 ({									\
 	typeof(pcp) ret__;						\
 	unsigned long flags;						\
-	local_irq_save(flags);						\
+	raw_local_irq_save(flags);						\
 	__this_cpu_add(pcp, val);					\
 	ret__ = __this_cpu_read(pcp);					\
-	local_irq_restore(flags);					\
+	raw_local_irq_restore(flags);					\
 	ret__;								\
 })
 
@@ -425,10 +425,10 @@ do {									\
 #define _this_cpu_generic_xchg(pcp, nval)				\
 ({	typeof(pcp) ret__;						\
 	unsigned long flags;						\
-	local_irq_save(flags);						\
+	raw_local_irq_save(flags);						\
 	ret__ = __this_cpu_read(pcp);					\
 	__this_cpu_write(pcp, nval);					\
-	local_irq_restore(flags);					\
+	raw_local_irq_restore(flags);					\
 	ret__;								\
 })
 
@@ -453,11 +453,11 @@ do {									\
 ({									\
 	typeof(pcp) ret__;						\
 	unsigned long flags;						\
-	local_irq_save(flags);						\
+	raw_local_irq_save(flags);						\
 	ret__ = __this_cpu_read(pcp);					\
 	if (ret__ == (oval))						\
 		__this_cpu_write(pcp, nval);				\
-	local_irq_restore(flags);					\
+	raw_local_irq_restore(flags);					\
 	ret__;								\
 })
 
@@ -490,10 +490,10 @@ do {									\
 ({									\
 	int ret__;							\
 	unsigned long flags;						\
-	local_irq_save(flags);						\
+	raw_local_irq_save(flags);						\
 	ret__ = __this_cpu_generic_cmpxchg_double(pcp1, pcp2,		\
 			oval1, oval2, nval1, nval2);			\
-	local_irq_restore(flags);					\
+	raw_local_irq_restore(flags);					\
 	ret__;								\
 })
 
-- 
1.7.9

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Christoph Lameter]

On Mon, 13 Feb 2012, Ming Lei wrote:

> It doesn't make sense to trace irq off or do irq flags
> lock proving inside 'this_cpu' operations, so replace local_irq_*
> with raw_local_irq_* in 'this_cpu' op.

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Tejun Heo]

On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
> It doesn't make sense to trace irq off or do irq flags
> lock proving inside 'this_cpu' operations, so replace local_irq_*
> with raw_local_irq_* in 'this_cpu' op.
> 
> Also the patch fixes one lockdep warning[1], which is caused
> by the added local_irq_save/restore(flags) in this_cpu_inc
> called by __debug_atomic_inc: kernel/lockdep.c

I think this isn't gonna hurt anything but I don't understand why the
lockdep warning is triggering when using traced version.  Can you
please explain that in a bit more detail in the patch description?

Thank you.

-- 
tejun

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Ming Lei]

Hi,

On Tue, Feb 14, 2012 at 1:23 AM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
>> It doesn't make sense to trace irq off or do irq flags
>> lock proving inside 'this_cpu' operations, so replace local_irq_*
>> with raw_local_irq_* in 'this_cpu' op.
>>
>> Also the patch fixes one lockdep warning[1], which is caused
>> by the added local_irq_save/restore(flags) in this_cpu_inc
>> called by __debug_atomic_inc: kernel/lockdep.c
>
> I think this isn't gonna hurt anything but I don't understand why the
> lockdep warning is triggering when using traced version.  Can you
> please explain that in a bit more detail in the patch description?

In trace_hardirqs_on_caller:kernel/lockdep.c, __debug_atomic_inc
will be called to add on 'this_cpu' variable, so may introduce recursive
trace_hardirqs_on|off_caller called.

For the lockdep warning, I reproduced it on ARM, see the path below:

kernel_thread_helper	/*irq disabled*/
	->entry: trace_hardirqs_on_caller	/*hardirqs_enabled was set*/
		->trace_hardirqs_off_caller /*hardirqs_enabled cleared*/
			__this_cpu_add(redundant_hardirqs_on)
		->trace_hardirqs_off_caller	/*irq disabled, so call here*/

so the 'unannotated irqs-on' warning will be triggered somewhere
because irq will be enabled just after the irq trace inside
kernel_thread_helper.
You can refer to log of commit  ac78884e6d89714d18b32b5b7d574116ecfb7c88
(ARM: lockdep: fix unannotated irqs-on) about irq trace inside
kernel_thread_helper.


thanks,
-- 
Ming Lei

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Yong Zhang]

On Tue, Feb 14, 2012 at 11:30:06AM +0800, Ming Lei wrote:
> Hi,
> 
> On Tue, Feb 14, 2012 at 1:23 AM, Tejun Heo <tj@kernel.org> wrote:
> > On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
> >> It doesn't make sense to trace irq off or do irq flags
> >> lock proving inside 'this_cpu' operations, so replace local_irq_*
> >> with raw_local_irq_* in 'this_cpu' op.
> >>
> >> Also the patch fixes one lockdep warning[1], which is caused
> >> by the added local_irq_save/restore(flags) in this_cpu_inc
> >> called by __debug_atomic_inc: kernel/lockdep.c
> >
> > I think this isn't gonna hurt anything but I don't understand why the
> > lockdep warning is triggering when using traced version. ?Can you
> > please explain that in a bit more detail in the patch description?
> 
> In trace_hardirqs_on_caller:kernel/lockdep.c, __debug_atomic_inc
> will be called to add on 'this_cpu' variable, so may introduce recursive
> trace_hardirqs_on|off_caller called.

Don't we need to prevent this kind of recursion first?

UNTESTED patch, I guess it'll smooth your concern.
---
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 8889f7d..028b4c5 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2561,6 +2561,8 @@ void trace_hardirqs_on_caller(unsigned long ip)
 	if (unlikely(!debug_locks || current->lockdep_recursion))
 		return;
 
+	current->lockdep_recursion = 1;
+
 	if (unlikely(current->hardirqs_enabled)) {
 		/*
 		 * Neither irq nor preemption are disabled here
@@ -2568,7 +2570,7 @@ void trace_hardirqs_on_caller(unsigned long ip)
 		 * in a stat is not a big deal.
 		 */
 		__debug_atomic_inc(redundant_hardirqs_on);
-		return;
+		goto out;
 	}
 
 	/*
@@ -2577,23 +2579,24 @@ void trace_hardirqs_on_caller(unsigned long ip)
 	 * enabled.. someone messed up their IRQ state tracing.
 	 */
 	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-		return;
+		goto out;
 
 	/*
 	 * See the fine text that goes along with this variable definition.
 	 */
 	if (DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled)))
-		return;
+		goto out;
 
 	/*
 	 * Can't allow enabling interrupts while in an interrupt handler,
 	 * that's general bad form and such. Recursion, limited stack etc..
 	 */
 	if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
-		return;
+		goto out;
 
-	current->lockdep_recursion = 1;
 	__trace_hardirqs_on_caller(ip);
+
+out:
 	current->lockdep_recursion = 0;
 }
 EXPORT_SYMBOL(trace_hardirqs_on_caller);

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Ming Lei]

On Tue, Feb 14, 2012 at 3:40 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Tue, Feb 14, 2012 at 11:30:06AM +0800, Ming Lei wrote:
>> Hi,
>>
>> On Tue, Feb 14, 2012 at 1:23 AM, Tejun Heo <tj@kernel.org> wrote:
>> > On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
>> >> It doesn't make sense to trace irq off or do irq flags
>> >> lock proving inside 'this_cpu' operations, so replace local_irq_*
>> >> with raw_local_irq_* in 'this_cpu' op.
>> >>
>> >> Also the patch fixes one lockdep warning[1], which is caused
>> >> by the added local_irq_save/restore(flags) in this_cpu_inc
>> >> called by __debug_atomic_inc: kernel/lockdep.c
>> >
>> > I think this isn't gonna hurt anything but I don't understand why the
>> > lockdep warning is triggering when using traced version. ?Can you
>> > please explain that in a bit more detail in the patch description?
>>
>> In trace_hardirqs_on_caller:kernel/lockdep.c, __debug_atomic_inc
>> will be called to add on 'this_cpu' variable, so may introduce recursive
>> trace_hardirqs_on|off_caller called.
>
> Don't we need to prevent this kind of recursion first?

IMO, lockdep is designed as not tracing or proving itself, and
just avoiding to trace __debug_atomic_inc is enough to fix the warning,
so it is not necessary to enlarge the protection range with
current->lockdep_recursion.

>
> UNTESTED patch, I guess it'll smooth your concern.
> ---
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index 8889f7d..028b4c5 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -2561,6 +2561,8 @@ void trace_hardirqs_on_caller(unsigned long ip)
>        if (unlikely(!debug_locks || current->lockdep_recursion))
>                return;
>
> +       current->lockdep_recursion = 1;
> +
>        if (unlikely(current->hardirqs_enabled)) {
>                /*
>                 * Neither irq nor preemption are disabled here
> @@ -2568,7 +2570,7 @@ void trace_hardirqs_on_caller(unsigned long ip)
>                 * in a stat is not a big deal.
>                 */
>                __debug_atomic_inc(redundant_hardirqs_on);
> -               return;
> +               goto out;
>        }
>
>        /*
> @@ -2577,23 +2579,24 @@ void trace_hardirqs_on_caller(unsigned long ip)
>         * enabled.. someone messed up their IRQ state tracing.
>         */
>        if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> -               return;
> +               goto out;
>
>        /*
>         * See the fine text that goes along with this variable definition.
>         */
>        if (DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled)))
> -               return;
> +               goto out;
>
>        /*
>         * Can't allow enabling interrupts while in an interrupt handler,
>         * that's general bad form and such. Recursion, limited stack etc..
>         */
>        if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
> -               return;
> +               goto out;
>
> -       current->lockdep_recursion = 1;
>        __trace_hardirqs_on_caller(ip);
> +
> +out:
>        current->lockdep_recursion = 0;
>  }
>  EXPORT_SYMBOL(trace_hardirqs_on_caller);


thanks,
-- 
Ming Lei

Re: [PATCH] percpu: use raw_local_irq_* in _this_cpu op

[Tejun Heo]

On Tue, Feb 14, 2012 at 11:30:06AM +0800, Ming Lei wrote:
> On Tue, Feb 14, 2012 at 1:23 AM, Tejun Heo <tj@kernel.org> wrote:
> > On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
> >> It doesn't make sense to trace irq off or do irq flags
> >> lock proving inside 'this_cpu' operations, so replace local_irq_*
> >> with raw_local_irq_* in 'this_cpu' op.
> >>
> >> Also the patch fixes one lockdep warning[1], which is caused
> >> by the added local_irq_save/restore(flags) in this_cpu_inc
> >> called by __debug_atomic_inc: kernel/lockdep.c
> >
> > I think this isn't gonna hurt anything but I don't understand why the
> > lockdep warning is triggering when using traced version.  Can you
> > please explain that in a bit more detail in the patch description?
> 
> In trace_hardirqs_on_caller:kernel/lockdep.c, __debug_atomic_inc
> will be called to add on 'this_cpu' variable, so may introduce recursive
> trace_hardirqs_on|off_caller called.

Ah, okay, so lockdep itself is using this_cpu ops.  Can you please
repost the patch with the above info in the description?

Thank you.

-- 
tejun