Skip to contentSkip to conversation

clockevent: don't remove broadcast device when cpu is dead

PATCH In review Xiaotian Feng 3 participants 9 messages Jan 7, 2010 – Jan 18, 2010

Subsystems (2)
High-resolution timers, timer wheel, clockevents Nohz, dynticks support
lore
Share & tools
lore

Xiaotian Feng

author
#0 lore raw

PATCH 1/1 clockevent: don't remove broadcast device when cpu is dead

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a broadcast device. We need to use tick_is_broadcast_device to check if it is a broadcast device.

Reported-by Marc Dionne <[email protected]>
Tested-by Marc Dionne <[email protected]>
Signed-off-by Xiaotian Feng <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Magnus Damm <[email protected]>
Cc H Hartley Sweeten <[email protected]>
---
 kernel/time/clockevents.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/time/clockevents.c +1 -1
index 6f740d9..0223d83 100644
@@ -259,7 +259,7 @@ void clockevents_notify(unsigned long reason, void *arg)
 		cpu = *((int *)arg);
 		list_for_each_entry_safe(dev, tmp, &clockevent_devices, list) {
 			if (cpumask_test_cpu(cpu, dev->cpumask) &&
-			    cpumask_weight(dev->cpumask) == 1) {
+			    !tick_is_broadcast_device(dev)) {
 				BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 				list_del(&dev->list);
 			}
Signature · 1 line
1.6.5.2
author
#1 lore raw

Hi:

Any comments on the patch? Marc confirmed this patch also fixed his
hang at suspend/resume stage. Thanks.

(Cc'ed [email protected])

Regards
Xiaotian

Quoted 32 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a broadcast device. We need to use tick_is_broadcast_device to check if it is a broadcast device.

Reported-by Marc Dionne<[email protected]>
Tested-by Marc Dionne<[email protected]>
Signed-off-by Xiaotian Feng<[email protected]>
Cc Thomas Gleixner<[email protected]>
Cc Magnus Damm<[email protected]>
Cc H Hartley Sweeten<[email protected]>
---
  kernel/time/clockevents.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
kernel/time/clockevents.c +1 -1
index 6f740d9..0223d83 100644
@@ -259,7 +259,7 @@ void clockevents_notify(unsigned long reason, void *arg)
  		cpu = *((int *)arg);
  		list_for_each_entry_safe(dev, tmp,&clockevent_devices, list) {
  			if (cpumask_test_cpu(cpu, dev->cpumask)&&
-			    cpumask_weight(dev->cpumask) == 1) {
+			    !tick_is_broadcast_device(dev)) {
  				BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
  				list_del(&dev->list);
  			}
#2 lore raw
In reply to Xiaotian Feng
Quoted 5 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

The patch is papering over the real problem.

Marc, can you please apply the following debug patch and provide the dmesg outputs from boot and shutdown ?

Thanks,

tglx
---

kernel/time/clockevents.c +9 -1
Index: linux-2.6-tip/kernel/time/clockevents.c
===================================================================
@@ -186,7 +186,7 @@ void clockevents_register_device(struct 
 	BUG_ON(!dev->cpumask);
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
-
+	printk(KERN_ERR "CE register %p %s\n", dev, dev->name);
 	list_add(&dev->list, &clockevent_devices);
 	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
 	clockevents_notify_released();

@@ -220,6 +220,7 @@ void clockevents_exchange_device(struct 
 	 * released list and do a notify add later.
 	 */
 	if (old) {
+		printk(KERN_INFO "CE Release %p %s\n", old, old->name);
 		clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED);
 		list_del(&old->list);
 		list_add(&old->list, &clockevents_released);

@@ -260,6 +261,13 @@ void clockevents_notify(unsigned long re
 		list_for_each_entry_safe(dev, tmp, &clockevent_devices, list) {
 			if (cpumask_test_cpu(cpu, dev->cpumask) &&
 			    cpumask_weight(dev->cpumask) == 1) {
+				if (dev->mode != CLOCK_EVT_MODE_UNUSED) {
+					printk(KERN_INFO
+					       "CE Remove %p %s bc: %d\n",
+					       dev, dev->name,
+					       tick_is_broadcast_device(dev));
+					continue;
+				}
 				BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 				list_del(&dev->list);
 			}
kernel/time/tick-broadcast.c +2 -0
Index: linux-2.6-tip/kernel/time/tick-broadcast.c
===================================================================
@@ -72,6 +72,8 @@ int tick_check_broadcast_device(struct c
 	     (dev->features & CLOCK_EVT_FEAT_C3STOP))
 		return 0;
 
+	printk(KERN_INFO "CE set broadcast %p %s\n", dev, dev->name);
+
 	clockevents_exchange_device(NULL, dev);
 	tick_broadcast_device.evtdev = dev;
 	if (!cpumask_empty(tick_get_broadcast_mask()))
#3 lore raw
Quoted 14 lines · Thomas Gleixner
Quoted 5 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

Quoted 8 lines

The patch is papering over the real problem.

Marc, can you please apply the following debug patch and provide the dmesg outputs from boot and shutdown ?

Thanks,

       tglx

Full dmesg output from boot is attached.

On shutdown I wrote down the message just before the BUG by hand (after modifying your patch a little to let it BUG):

CE Remove ffffffff81a44100 hpet bc: 1

Marc

author
#4 lore raw
In reply to Thomas Gleixner
Quoted 5 lines · Thomas Gleixner
Quoted 3 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

Quoted 8 lines
Quoted 2 lines

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

At least for hpet broadcast dev, it's dev->cpumask is only contain the CPU which it is initialized from. And for broadcast device, kernel is using tick_broadcast_mask not dev->cpumask, right?

Quoted 60 lines

The patch is papering over the real problem.

Marc, can you please apply the following debug patch and provide the dmesg outputs from boot and shutdown ?

Thanks,

tglx
---

Index: linux-2.6-tip/kernel/time/clockevents.c
===================================================================

kernel/time/clockevents.c +9 -1
@@ -186,7 +186,7 @@ void clockevents_register_device(struct
  	BUG_ON(!dev->cpumask);
 
  	raw_spin_lock_irqsave(&clockevents_lock, flags);
-
+	printk(KERN_ERR "CE register %p %s\n", dev, dev->name);
  	list_add(&dev->list,&clockevent_devices);
  	clockevents_do_notify(CLOCK_EVT_NOTIFY_ADD, dev);
  	clockevents_notify_released();

@@ -220,6 +220,7 @@ void clockevents_exchange_device(struct
  	 * released list and do a notify add later.
  	 */
  	if (old) {
+		printk(KERN_INFO "CE Release %p %s\n", old, old->name);
  		clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED);
  		list_del(&old->list);
  		list_add(&old->list,&clockevents_released);

@@ -260,6 +261,13 @@ void clockevents_notify(unsigned long re
  		list_for_each_entry_safe(dev, tmp,&clockevent_devices, list) {
  			if (cpumask_test_cpu(cpu, dev->cpumask)&&
  			cpumask_weight(dev->cpumask) == 1) {
+				if (dev->mode != CLOCK_EVT_MODE_UNUSED) {
+					printk(KERN_INFO
+					       "CE Remove %p %s bc: %d\n",
+					       dev, dev->name,
+					       tick_is_broadcast_device(dev));
+					continue;
+				}
  				BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
  				list_del(&dev->list);
  			}

Index: linux-2.6-tip/kernel/time/tick-broadcast.c
===================================================================

kernel/time/tick-broadcast.c +2 -0
@@ -72,6 +72,8 @@ int tick_check_broadcast_device(struct c
  	     (dev->features&  CLOCK_EVT_FEAT_C3STOP))
  		return 0;
 
+	printk(KERN_INFO "CE set broadcast %p %s\n", dev, dev->name);
+
  	clockevents_exchange_device(NULL, dev);
  	tick_broadcast_device.evtdev = dev;
  	if (!cpumask_empty(tick_get_broadcast_mask()))
#5 lore raw
Quoted 9 lines · Xiaotian Feng
Quoted 8 lines · Thomas Gleixner
Quoted 5 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Quoted 8 lines
Quoted 5 lines

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

At least for hpet broadcast dev, it's dev->cpumask is only contain the CPU which it is initialized from.

Which is fundamentaly wrong and the root cause of the problem. I'll have a look tomorrow morning when my brain is more awake than now.

Quoted 2 lines

And for broadcast device, kernel is using tick_broadcast_mask not dev->cpumask, right?

No, tick_broadcast_mask is the bitmask which tells us which cpus get the broadcast IPI.

Thanks,

tglx

author
#6 lore raw
Quoted 13 lines · Thomas Gleixner
Quoted 12 lines · Xiaotian Feng
Quoted 11 lines · Thomas Gleixner
Quoted 5 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

At least for hpet broadcast dev, it's dev->cpumask is only contain the CPU which it is initialized from.

Which is fundamentaly wrong and the root cause of the problem. I'll have a look tomorrow morning when my brain is more awake than now.

hpet_legacy_clockevent_register is trying to register new CE, but replace failed, then in tick_check_new_device -> tick_check_broadcast_device, the legacy hpet CE was registered as multicast device, but its dev->cpumask is cpumask of smp_processor_id().

on my system its dev->cpumask is cpumask of 0, but in Marc's, dev->cpumask is cpumask of 4. So when kernel is trying to offline cpu 4, the broadcast hpet is removed.

Quoted 11 lines
Quoted 3 lines

And for broadcast device, kernel is using tick_broadcast_mask not dev->cpumask, right?

No, tick_broadcast_mask is the bitmask which tells us which cpus get the broadcast IPI.

Thanks,

tglx

#7 lore raw
Quoted 30 lines · Xiaotian Feng
Quoted 13 lines · Thomas Gleixner
Quoted 12 lines · Xiaotian Feng
Quoted 11 lines · Thomas Gleixner
Quoted 5 lines · Xiaotian Feng

Marc reported BUG during shutdown, after debugging, kernel is trying to remove a broadcast device which mode is CLOCK_EVT_MODE_ONESHOT.

The root cause for this bug is that in clockevents_notify, "cpumask_weight(dev->cpumask) == 1" is always true even if dev is a

Why is cpumask_weight(dev->cpumask) == 1 always true when we shutdown a non boot cpu ?

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The broadcast device is not a per cpu device and the cpumask should not only contain the CPU which is shut down !

At least for hpet broadcast dev, it's dev->cpumask is only contain the CPU which it is initialized from.

Which is fundamentaly wrong and the root cause of the problem. I'll have a look tomorrow morning when my brain is more awake than now.

hpet_legacy_clockevent_register is trying to register new CE, but replace failed,
then in tick_check_new_device -> tick_check_broadcast_device, the legacy hpet CE
was registered as multicast device, but its dev->cpumask is cpumask of smp_processor_id().

Quoted 3 lines

on my system its dev->cpumask is cpumask of 0, but in Marc's, dev->cpumask is cpumask of 4.
So when kernel is trying to offline cpu 4, the broadcast hpet is removed.

I know, but the point is that a broadcast device should not be bound to the CPU which is registering it. Working on a fix.

Thanks,

tglx

author
#8 lore raw

[tip:timers/urgent] clockevent: Don't remove broadcast device when cpu is dead

In reply to Xiaotian Feng
Commit-ID:  ea9d8e3f45404d411c00ae67b45cc35c58265bb7
Gitweb:     http://git.kernel.org/tip/ea9d8e3f45404d411c00ae67b45cc35c58265bb7
Author:     Xiaotian Feng <[email protected]>
AuthorDate: Thu, 7 Jan 2010 11:22:44 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 18 Jan 2010 14:44:50 +0100

clockevent: Don't remove broadcast device when cpu is dead

Marc reported that the BUG_ON in clockevents_notify() triggers on his system. This happens because the kernel tries to remove an active clock event device (used for broadcasting) from the device list.

The handling of devices which can be used as per cpu device and as a global broadcast device is suboptimal.

The simplest solution for now (and for stable) is to check whether the device is used as global broadcast device, but this needs to be revisited.

[ tglx: restored the cpuweight check and massaged the changelog ]

Reported-by Marc Dionne <[email protected]>
Tested-by Marc Dionne <[email protected]>
Signed-off-by Xiaotian Feng <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>
Cc [email protected]
---
 kernel/time/clockevents.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
kernel/time/clockevents.c +2 -1
index 6f740d9..d7395fd 100644
@@ -259,7 +259,8 @@ void clockevents_notify(unsigned long reason, void *arg)
 		cpu = *((int *)arg);
 		list_for_each_entry_safe(dev, tmp, &clockevent_devices, list) {
 			if (cpumask_test_cpu(cpu, dev->cpumask) &&
-			    cpumask_weight(dev->cpumask) == 1) {
+			    cpumask_weight(dev->cpumask) == 1 &&
+			    !tick_is_broadcast_device(dev)) {
 				BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
 				list_del(&dev->list);
 			}