oprofile: add SMP barriers for hrtimer hotplug code

PATCH In review Will Deacon 3 participants 4 messages Jan 31, 2011 – Feb 1, 2011

Share & tools

Will Deacon

author

2011-01-31 17:53 UTC #0 lore raw

PATCH v2 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 an smp_mb() before initialising the hrtimers 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]>

---

Changes since v1: Removed redundant barrier from stop() code.

 drivers/oprofile/timer_int.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/oprofile/timer_int.c +1 -0

index 0107251..a3a94ea 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;

Signature · 1 line

1.7.0.4

Santosh Shilimkar

Acked-by on v2 1/1

2011-02-01 05:27 UTC #1 lore raw

Quoted 26 lines

-----Original Message-----
From: Will Deacon [mailto:[email protected]]
Sent: Monday, January 31, 2011 11:23 PM
To: [email protected]; [email protected]
Cc: Will Deacon; Santosh Shilimkar; Robert Richter
Subject: [PATCH v2] 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.

Quoted 9 lines

This patch adds an smp_mb() before initialising the hrtimers 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]>
---

Acked-by Santosh Shilimkar <[email protected]>

Quoted 21 lines

Changes since v1: Removed redundant barrier from stop() code.

 drivers/oprofile/timer_int.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/oprofile/timer_int.c +1 -1

index 0107251..a3a94ea 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;
--

1.7.0.4

Ming Lei

2011-02-01 08:03 UTC #2 lore raw

Hi,

2011/2/1 Santosh Shilimkar <[email protected]>:

Quoted 5 lines

Quoted 5 lines
-----Original Message-----
From: Will Deacon [mailto:[email protected]]
drivers/oprofile/timer_int.c +0 -0
index 0107251..a3a94ea 100644

Quoted 8 lines

drivers/oprofile/timer_int.c static int oprofile_hrtimer_start(void) · line 52
Quoted 8 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();
       on_each_cpu(__oprofile_hrtimer_start, NULL, 1);

Just a little question:

Before calling __oprofile_hrtimer_start in other CPUs, there have been some smp_mb executed already, such as called by smp_call_function_many in current CPU, and called by generic_smp_call_function_interrupt in the func-calling CPU, so are these smp_mb enough for correct order of access for 'ctr_running' ?

Quoted 2 lines

drivers/oprofile/timer_int.c static int oprofile_hrtimer_start(void) · line 54
Quoted 2 lines
drivers/oprofile/timer_int.c +0 -0
       put_online_cpus();
       return 0;

thanks,

Signature · 1 line

Lei Ming

Will Deacon

author

2011-02-01 17:08 UTC #3 lore raw

Quoted 2 lines · Ming Lei

Hi,

Hello Lei Ming,

Quoted 17 lines

2011/2/1 Santosh Shilimkar <[email protected]>:
Quoted 5 lines
Quoted 5 lines
-----Original Message-----
From: Will Deacon [mailto:[email protected]]
drivers/oprofile/timer_int.c +0 -0
index 0107251..a3a94ea 100644
Quoted 8 lines
drivers/oprofile/timer_int.c static int oprofile_hrtimer_start(void) · line 52
Quoted 8 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();
      on_each_cpu(__oprofile_hrtimer_start, NULL, 1);
Just a little question:

Quoted 6 lines

Before calling __oprofile_hrtimer_start in other CPUs, there have been some smp_mb executed already, such as called by smp_call_function_many in current CPU, and called by generic_smp_call_function_interrupt in the func-calling CPU, so are these smp_mb enough for correct order of access for 'ctr_running' ?

I didn't dive into the guts of the cross-calling implementation, but it appears that you're correct. There's also the csd_{un}lock functions which have smp_mb() in them too, so I think we have plenty of barrier goodness as it is.

I'll trash the patch.

Cheers,

Will