Skip to contentSkip to conversation

oprofile: add SMP barriers for hrtimer hotplug code

PATCH Superseded Will Deacon 2 participants 3 messages Jan 31, 2011

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Will Deacon

author
#0 lore raw

PATCH 1/1 oprofile: add SMP barriers for hrtimer hotplug code

OProfile uses a CPU notifier to start and stop any hrtimers when CPUs change between ONLINE and DEAD. A static int ctr_running is used to keep track of the counter state.

This can lead to problems where writes to the state variable are re-ordered with repect to reads of the variable occurring on other CPUs, meaning that __oprofile_hrtimer_start may read ctr_running as 0 and not initialise the hrtimer. Potential deadlock can occur in __oprofile_hrtimer_stop because lock_hrtimer_base will poll until timer->base != NULL, which will never happen.

This patch adds smp_mb()s to ensure that ctr_running mirrors the correct counter state.

Cc Santosh Shilimkar <[email protected]>
Cc Robert Richter <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 drivers/oprofile/timer_int.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/oprofile/timer_int.c +2 -0
index 0107251..38c1e1b 100644
@@ -48,6 +48,7 @@ static int oprofile_hrtimer_start(void)
 {
 	get_online_cpus();
 	ctr_running = 1;
+	smp_mb();
 	on_each_cpu(__oprofile_hrtimer_start, NULL, 1);
 	put_online_cpus();
 	return 0;

@@ -70,6 +71,7 @@ static void oprofile_hrtimer_stop(void)
 	get_online_cpus();
 	for_each_online_cpu(cpu)
 		__oprofile_hrtimer_stop(cpu);
+	smp_mb();
 	ctr_running = 0;
 	put_online_cpus();
 }
Signature · 1 line
1.7.0.4
Acked-by on v1 1/1
#1 lore raw
Quoted 42 lines

-----Original Message-----
From: Will Deacon [mailto:[email protected]]
Sent: Monday, January 31, 2011 8:38 PM
To: [email protected]; [email protected]
Cc: Will Deacon; Santosh Shilimkar; Robert Richter
Subject: [PATCH] oprofile: add SMP barriers for hrtimer hotplug code

OProfile uses a CPU notifier to start and stop any hrtimers when CPUs change
between ONLINE and DEAD. A static int ctr_running is used to keep track of
the counter state.

This can lead to problems where writes to the state variable are re- ordered
with repect to reads of the variable occurring on other CPUs, meaning that
__oprofile_hrtimer_start may read ctr_running as 0 and not initialise the
hrtimer. Potential deadlock can occur in __oprofile_hrtimer_stop because
lock_hrtimer_base will poll until timer->base != NULL, which will never
happen.

This patch adds smp_mb()s to ensure that ctr_running mirrors the correct
counter state.

Cc Santosh Shilimkar <[email protected]>
Cc Robert Richter <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 drivers/oprofile/timer_int.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/oprofile/timer_int.c +0 -0
index 0107251..38c1e1b 100644

drivers/oprofile/timer_int.c static int oprofile_hrtimer_start(void) · line 51

Quoted 4 lines
drivers/oprofile/timer_int.c +1 -0
@@ -48,6 +48,7 @@ static int oprofile_hrtimer_start(void)
 {
 	get_online_cpus();
 	ctr_running = 1;
+	smp_mb();

Is smp_wmb() more appropriate ?

drivers/oprofile/timer_int.c static void oprofile_hrtimer_stop(void)

Quoted 12 lines
drivers/oprofile/timer_int.c +1 -1
 	on_each_cpu(__oprofile_hrtimer_start, NULL, 1);
 	put_online_cpus();
 	return 0;

@@ -70,6 +71,7 @@ static void oprofile_hrtimer_stop(void)
 	get_online_cpus();
 	for_each_online_cpu(cpu)
 		__oprofile_hrtimer_stop(cpu);
+	smp_mb();
 	ctr_running = 0;
 	put_online_cpus();
 }
--

Otherwise patch is fine.

Acked-by Santosh Shilimkar <[email protected]>
author
#2 lore raw

Hi Santosh,

Quoted 12 lines · Santosh Shilimkar

drivers/oprofile/timer_int.c static int oprofile_hrtimer_start(void) · line 51

Quoted 10 lines
drivers/oprofile/timer_int.c +1 -0
index 0107251..38c1e1b 100644
@@ -48,6 +48,7 @@ static int oprofile_hrtimer_start(void)
 {
      get_online_cpus();
      ctr_running = 1;
+     smp_mb();

Is smp_wmb() more appropriate ?

We could use smp_wmb, but then we have to add smp_rmb before reading ctr_running too.

Quoted 11 lines

drivers/oprofile/timer_int.c static void oprofile_hrtimer_stop(void) · line 77

Quoted 11 lines
drivers/oprofile/timer_int.c +1 -0
      on_each_cpu(__oprofile_hrtimer_start, NULL, 1);
      put_online_cpus();
      return 0;

@@ -70,6 +71,7 @@ static void oprofile_hrtimer_stop(void)
      get_online_cpus();
      for_each_online_cpu(cpu)
              __oprofile_hrtimer_stop(cpu);
+     smp_mb();
      ctr_running = 0;
      put_online_cpus();
 }

I misread this code: it actually all executes on the current CPU so we don't need the barrier. The control hazard is enough.

Quoted 3 lines
Quoted 1 line

--

Otherwise patch is fine.

Acked-by Santosh Shilimkar <[email protected]>

Ok, thanks. I will post a v2 anyway with the second barrier removed. Feel free to re-ack!

Will