Patchwork powerpc: Add write barrier before enabling DTL flags

login
register
mail settings
Submitter Jeremy Kerr
Date March 24, 2009, 2:55 a.m.
Message ID <1237863308.620048.912148989400.1.gpush@pingu>
Download mbox | patch
Permalink /patch/24936/
State Accepted
Commit 82631f5dd114e52239fb3d1e270a49d37c088b46
Delegated to: Paul Mackerras
Headers show

Comments

Jeremy Kerr - March 24, 2009, 2:55 a.m.
Currently, we don't enforce any ordering for updates to the lppaca
when enabling dtl logging, so we may end up enabling logging before the
index fields have been established.

This change adds a smp_wmb() before doing the actual enable.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 arch/powerpc/platforms/pseries/dtl.c |    4 ++++
 1 file changed, 4 insertions(+)
Michael Ellerman - March 24, 2009, 5:53 a.m.
On Tue, 2009-03-24 at 13:55 +1100, Jeremy Kerr wrote:
> Currently, we don't enforce any ordering for updates to the lppaca
> when enabling dtl logging, so we may end up enabling logging before the
> index fields have been established.
> 
> This change adds a smp_wmb() before doing the actual enable.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> 
> ---
>  arch/powerpc/platforms/pseries/dtl.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
> index dc9b0f8..fafcaa0 100644
> --- a/arch/powerpc/platforms/pseries/dtl.c
> +++ b/arch/powerpc/platforms/pseries/dtl.c
> @@ -107,6 +107,10 @@ static int dtl_enable(struct dtl *dtl)
>  	/* set our initial buffer indices */
>  	dtl->last_idx = lppaca[dtl->cpu].dtl_idx = 0;
>  
> +	/* ensure that our updates to the lppaca fields have occurred before
> +	 * we actually enable the logging */
> +	smp_wmb();

Wouldn't this still be a problem on a UP kernel?

cheers
Jeremy Kerr - March 24, 2009, 5:59 a.m.
Michael,

> Wouldn't this still be a problem on a UP kernel?

I don't believe so - stores should be ordered with respect to the 
current CPU, and in the UP case we still get a barrier().

However, perhaps there are other considerations with the HV that I'm not 
aware of. Anyone?

Cheers,


Jeremy
Michael Ellerman - March 25, 2009, 12:44 a.m.
On Tue, 2009-03-24 at 16:59 +1100, Jeremy Kerr wrote:
> Michael,
> 
> > Wouldn't this still be a problem on a UP kernel?
> 
> I don't believe so - stores should be ordered with respect to the 
> current CPU, and in the UP case we still get a barrier().

But what if the CPU decides to do the store to the enable_mask before
the stores to the other fields?

cheers

Patch

diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
index dc9b0f8..fafcaa0 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -107,6 +107,10 @@  static int dtl_enable(struct dtl *dtl)
 	/* set our initial buffer indices */
 	dtl->last_idx = lppaca[dtl->cpu].dtl_idx = 0;
 
+	/* ensure that our updates to the lppaca fields have occurred before
+	 * we actually enable the logging */
+	smp_wmb();
+
 	/* enable event logging */
 	lppaca[dtl->cpu].dtl_enable_mask = dtl_event_mask;