[PATCH] PPC64: lockfix for rtas error log

[PATCH] PPC64: lockfix for rtas error log

[linas]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]


Paul, 

Could you please apply the following path to the ameslab tree, and/or 
forward it to the main 2.6 kernel maintainers.

This patch moves the location of a lock in order to protect
the contents of a buffer until it has been copied to its final
destination. Prior to this, a race existed whereby the buffer
could be filled even while it was being emptied.

Signed-off-by: Linas Vepstas <linas@linas.org>

--linas


[-- Attachment #2: rtas-erbbuf-lockfix.patch --]
[-- Type: text/plain, Size: 857 bytes --]

--- arch/ppc64/kernel/rtas.c.orig-pre-lockfix	2004-06-29 17:02:12.000000000 -0500
+++ arch/ppc64/kernel/rtas.c	2004-06-29 17:14:05.000000000 -0500
@@ -134,9 +134,10 @@ log_rtas_error(void)
 
 	spin_lock_irqsave(&rtas.lock, s);
 	rc = __log_rtas_error();
-	spin_unlock_irqrestore(&rtas.lock, s);
-	if (rc == 0)
+	if (rc == 0) {
 		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
+	}
+	spin_unlock_irqrestore(&rtas.lock, s);
 }
 
 int
@@ -193,12 +194,13 @@ rtas_call(int token, int nargs, int nret
 			outputs[i] = rtas_args->rets[i+1];
 	ret = (int)((nret > 0) ? rtas_args->rets[0] : 0);
 
+	if (logit) {
+		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
+	}
+	
 	/* Gotta do something different here, use global lock for now... */
 	spin_unlock_irqrestore(&rtas.lock, s);
 
-	if (logit)
-		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
-
 	return ret;
 }
 

Re: [PATCH] PPC64: lockfix for rtas error log

[David Gibson]

On Tue, Jun 29, 2004 at 05:50:07PM -0500, linas@austin.ibm.com wrote:
> 
> Paul,
> 
> Could you please apply the following path to the ameslab tree, and/or
> forward it to the main 2.6 kernel maintainers.
> 
> This patch moves the location of a lock in order to protect
> the contents of a buffer until it has been copied to its final
> destination. Prior to this, a race existed whereby the buffer
> could be filled even while it was being emptied.
> 
> Signed-off-by: Linas Vepstas <linas@linas.org>

Hmm... I note that log_error() does nothing but call ppc_md.log_error,
and I can't see anywhere that that is set to be non-NULL...

> +++ arch/ppc64/kernel/rtas.c	2004-06-29 17:14:05.000000000 -0500
> @@ -134,9 +134,10 @@ log_rtas_error(void)
> 
>  	spin_lock_irqsave(&rtas.lock, s);
>  	rc = __log_rtas_error();
> -	spin_unlock_irqrestore(&rtas.lock, s);
> -	if (rc == 0)
> +	if (rc == 0) {
>  		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
> +	}
> +	spin_unlock_irqrestore(&rtas.lock, s);
>  }
> 
>  int
> @@ -193,12 +194,13 @@ rtas_call(int token, int nargs, int nret
>  			outputs[i] = rtas_args->rets[i+1];
>  	ret = (int)((nret > 0) ? rtas_args->rets[0] : 0);
> 
> +	if (logit) {
> +		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
> +	}
> +
>  	/* Gotta do something different here, use global lock for now... */
>  	spin_unlock_irqrestore(&rtas.lock, s);
> 
> -	if (logit)
> -		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
> -
>  	return ret;
>  }
> 


-- 
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: [PATCH] PPC64: lockfix for rtas error log

[Benjamin Herrenschmidt]

On Tue, 2004-06-29 at 17:50, linas@austin.ibm.com wrote:
> Paul,
> 
> Could you please apply the following path to the ameslab tree, and/or
> forward it to the main 2.6 kernel maintainers.
> 
> This patch moves the location of a lock in order to protect
> the contents of a buffer until it has been copied to its final
> destination. Prior to this, a race existed whereby the buffer
> could be filled even while it was being emptied.

Hrm....

That's bad, I moved that out of the lock on purpose to avoid deadlocks,
I think ppc_md.log_error can take the rtas lock again (nvram). We need to
take a separate lock for the err buf if that function can be called
concurrently I suppose.

Ben.

Re: [PATCH] PPC64: lockfix for rtas error log

[Paul Mackerras]

Linas,

> This patch moves the location of a lock in order to protect
> the contents of a buffer until it has been copied to its final
> destination. Prior to this, a race existed whereby the buffer
> could be filled even while it was being emptied.

Given that log_error() seems to be a no-op at the moment AFAICT, and
that Ben H was concerned about possible deadlocks if log_error
actually did do something, I'd like to see a resolution of (a) what
log_error should be doing and (b) whether there is in fact any
possibility of deadlock with this patch once (a) is resolved.

Thanks,
Paul.

Re: [PATCH] PPC64: lockfix for rtas error log

[linas]

On Wed, Jun 30, 2004 at 11:17:08AM +1000, David Gibson wrote:
> On Tue, Jun 29, 2004 at 05:50:07PM -0500, linas@austin.ibm.com wrote:
> > 
> > Paul,
> > 
> > Could you please apply the following path to the ameslab tree, and/or
> > forward it to the main 2.6 kernel maintainers.
> > 
> > This patch moves the location of a lock in order to protect
> > the contents of a buffer until it has been copied to its final
> > destination. Prior to this, a race existed whereby the buffer
> > could be filled even while it was being emptied.
> > 
> > Signed-off-by: Linas Vepstas <linas@linas.org>
> 
> Hmm... I note that log_error() does nothing but call ppc_md.log_error,
> and I can't see anywhere that that is set to be non-NULL...

grep log_error *.c
chrp_setup.c:   ppc_md.log_error = pSeries_log_error;

--linas

Re: [PATCH] PPC64: lockfix for rtas error log

[linas]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On Tue, Jun 29, 2004 at 08:44:26PM -0500, Benjamin Herrenschmidt wrote:
> On Tue, 2004-06-29 at 17:50, linas@austin.ibm.com wrote:
> > Paul,
> > 
> > Could you please apply the following path to the ameslab tree, and/or
> > forward it to the main 2.6 kernel maintainers.
> > 
> > This patch moves the location of a lock in order to protect
> > the contents of a buffer until it has been copied to its final
> > destination. Prior to this, a race existed whereby the buffer
> > could be filled even while it was being emptied.
> 
> Hrm....
> 
> That's bad, I moved that out of the lock on purpose to avoid deadlocks,

I looked for deadlocks, but didn't see anything obvious.  I'll take your
word, though.

> I think ppc_md.log_error can take the rtas lock again (nvram). We need to
> take a separate lock for the err buf if that function can be called
> concurrently I suppose.

Well, the problem was that there is no lock that is protecting the 
use of the single, global buffer.  Adding yet another lock is bad;
it makes hunting for deadlocks that much more tedious and difficult;
already, finding deadlocks is error-prone, and subject to bit-rot as 
future hackers update the code.  So instead, the problem can be easily 
avoided by not using a global buffer.  The code below mallocs/frees.
Its not perf-critcal, so I don't mind malloc overhead.  Would this
work for you?  Patch attached below.

Signed-off-by: Linas Vepstas <linas@linas.org>

--linas




[-- Attachment #2: rtas-lock-malloc.patch --]
[-- Type: text/plain, Size: 1458 bytes --]

--- arch/ppc64/kernel/rtas.c.orig-pre-lockfix	2004-06-29 17:02:12.000000000 -0500
+++ arch/ppc64/kernel/rtas.c	2004-06-30 12:26:51.000000000 -0500
@@ -131,12 +131,20 @@ log_rtas_error(void)
 {
 	unsigned long s;
 	int rc;
+	char * buff_copy = NULL;
 
 	spin_lock_irqsave(&rtas.lock, s);
 	rc = __log_rtas_error();
+	if (rc == 0) {
+		buff_copy = kmalloc (RTAS_ERROR_LOG_MAX, GFP_ATOMIC);
+		memcpy (buff_copy, rtas_err_buf, RTAS_ERROR_LOG_MAX);
+	}
 	spin_unlock_irqrestore(&rtas.lock, s);
-	if (rc == 0)
-		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
+
+	if (buff_copy) {
+		log_error(buff_copy, ERR_TYPE_RTAS_LOG, 0);
+		kfree (buff_copy);
+	}
 }
 
 int
@@ -147,6 +155,7 @@ rtas_call(int token, int nargs, int nret
 	int i, logit = 0;
 	unsigned long s;
 	struct rtas_args *rtas_args;
+	char * buff_copy = NULL;
 	int ret;
 
 	PPCDBG(PPCDBG_RTAS, "Entering rtas_call\n");
@@ -193,12 +202,18 @@ rtas_call(int token, int nargs, int nret
 			outputs[i] = rtas_args->rets[i+1];
 	ret = (int)((nret > 0) ? rtas_args->rets[0] : 0);
 
+	if (logit) {
+		buff_copy = kmalloc (RTAS_ERROR_LOG_MAX, GFP_ATOMIC);
+		memcpy (buff_copy, rtas_err_buf, RTAS_ERROR_LOG_MAX);
+	}
+	
 	/* Gotta do something different here, use global lock for now... */
 	spin_unlock_irqrestore(&rtas.lock, s);
 
-	if (logit)
-		log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0);
-
+	if (buff_copy) {
+		log_error(buff_copy, ERR_TYPE_RTAS_LOG, 0);
+		kfree (buff_copy);
+	}
 	return ret;
 }
 

Re: [PATCH] PPC64: lockfix for rtas error log

[linas]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Wed, Jun 30, 2004 at 09:27:09PM +1000, Paul Mackerras wrote:
> Linas,
> 
> > This patch moves the location of a lock in order to protect
> > the contents of a buffer until it has been copied to its final
> > destination. Prior to this, a race existed whereby the buffer
> > could be filled even while it was being emptied.
> 
> Given that log_error() seems to be a no-op at the moment AFAICT, and

!!
Too many kernel trees.  Someone must have whacked the ameslab tree
by accident, or on purpose, because they got sick of seeing rtas 
messages.  I don't find the RTAS messages to be pleasent, but simply
whacking them is not the right solution.  The following diff is 
between an older tree and the current tree.  If you could re-add 
these lines, that would be great.

--linas


[-- Attachment #2: chrp-setup.patch --]
[-- Type: text/plain, Size: 491 bytes --]

--- ameslab-2.6.7-25june/arch/ppc64/kernel/chrp_setup.c	2004-06-29 17:04:30.000000000 -0500
+++ linux-2.6.5-20040514/arch/ppc64/kernel/chrp_setup.c	2004-06-29 17:28:48.000000000 -0500
@@ -243,6 +243,12 @@ chrp_init(unsigned long r3, unsigned lon
 		ppc_md.get_irq        = xics_get_irq;
 	}
 
+#ifdef CONFIG_PPC_PSERIES
+	ppc_md.log_error = pSeries_log_error;
+#else
+	ppc_md.log_error = NULL;
+#endif
+
 	ppc_md.init           = chrp_init2;
 
 	ppc_md.pcibios_fixup  = pSeries_final_fixup;

Re: [PATCH] PPC64: lockfix for rtas error log

[Benjamin Herrenschmidt]

> Well, the problem was that there is no lock that is protecting the
> use of the single, global buffer.  Adding yet another lock is bad;
> it makes hunting for deadlocks that much more tedious and difficult;
> already, finding deadlocks is error-prone, and subject to bit-rot as
> future hackers update the code.  So instead, the problem can be easily
> avoided by not using a global buffer.  The code below mallocs/frees.
> Its not perf-critcal, so I don't mind malloc overhead.  Would this
> work for you?  Patch attached below.

I prefer that, but couldn't we move the kmalloc outside of the spinlock
and so use GFP_KERNEL instead ?

Ben.

Re: [PATCH] PPC64: lockfix for rtas error log

[Benjamin Herrenschmidt]

> Isn't the global buffer used since it's in BSS, and as such, in low 
> memory, guaranteed to be below 4GB? RTAS has limitations that restricts 
> any passed-in buffers to be addressable as 32-bit real mode pointers, right?

The global buffer is still used within the spinlocked region for RTAS itself,
the patch just uses a local buffer and copies into it from the global one
before releasing the lock.

Ben.

Re: [PATCH] PPC64: lockfix for rtas error log

[Olof Johansson]

Benjamin Herrenschmidt wrote:
>> So instead, the problem can be easily
>>avoided by not using a global buffer.  The code below mallocs/frees.
>>Its not perf-critcal, so I don't mind malloc overhead.  Would this
>>work for you?  Patch attached below.
> 
> 
> I prefer that, but couldn't we move the kmalloc outside of the spinlock
> and so use GFP_KERNEL instead ?

Isn't the global buffer used since it's in BSS, and as such, in low 
memory, guaranteed to be below 4GB? RTAS has limitations that restricts 
any passed-in buffers to be addressable as 32-bit real mode pointers, right?


-Olof

[PATCH] 2.6 PPC64: lockfix for rtas error log (third-times-a-charm?)

[linas]

On Wed, Jun 30, 2004 at 01:47:29PM -0500, Benjamin Herrenschmidt wrote:
> 
> > Well, the problem was that there is no lock that is protecting the
> > use of the single, global buffer.  Adding yet another lock is bad;
> > it makes hunting for deadlocks that much more tedious and difficult;
> > already, finding deadlocks is error-prone, and subject to bit-rot as
> > future hackers update the code.  So instead, the problem can be easily
> > avoided by not using a global buffer.  The code below mallocs/frees.
> > Its not perf-critcal, so I don't mind malloc overhead.  Would this
> > work for you?  Patch attached below.
> 
> I prefer that, but couldn't we move the kmalloc outside of the spinlock
> and so use GFP_KERNEL instead ?

OK,

Upon closer analysis of the code, I see that log_rtas_error() 
was incorrectly named, and was being used incorrectly.  The 
solution is to get rid of it entirely; see patch below. So:

-- In one case kmalloc must be GFP_ATOMIC because rtas_call()
   can happen in any context, incl. irqs.
-- In the other case, I turned it into GFP_KENREL, at the cost
   of doing a needless malloc/free in the vast majority of 
   cases where there is no error.  Small price, as I beleive
   that this routine is very rarely called.

Patch below, 
Signed-off-by: Linas Vepstas <linas@linas.org>

--linas

Re: [PATCH] PPC64: lockfix for rtas error log

[Paul Mackerras]

Linas,

> Too many kernel trees.  Someone must have whacked the ameslab tree
> by accident, or on purpose, because they got sick of seeing rtas 
> messages.  I don't find the RTAS messages to be pleasent, but simply
> whacking them is not the right solution.  The following diff is 
> between an older tree and the current tree.  If you could re-add 
> these lines, that would be great.

As far as I can see, the ameslab tree has _never_ contained those
lines.  The last change to chrp_setup.c was on 1 May 2004, and neither
that version nor any of the earlier versions that I looked at have
those lines.  Are you sure you don't have that as a local change in
your copy?  Do a bk sfiles -i and a bk push -n and see if it shows up.

In any case the #ifdef is redundant, because chrp_setup.o is only
linked in if CONFIG_PPC_PSERIES is set.

As for the RTAS messages being printk'd, the only possible
justification would be if there was a userspace tool to analyse them.
I don't know if such a thing exists, and if it does, I certainly don't
have a copy.  Is anyone working on that?

Paul.

Re: [PATCH] PPC64: lockfix for rtas error log

[Jake Moilanen]

> As far as I can see, the ameslab tree has _never_ contained those
> lines.  The last change to chrp_setup.c was on 1 May 2004, and neither
> that version nor any of the earlier versions that I looked at have
> those lines.  Are you sure you don't have that as a local change in
> your copy?  Do a bk sfiles -i and a bk push -n and see if it shows up.

Back on May 6th, Nathan Fontenot posted a patch to linuxppc64, that had
this fixed when he noticed it got removed.  It looks like the patch was
never merged.
 
> As for the RTAS messages being printk'd, the only possible
> justification would be if there was a userspace tool to analyse them.
> I don't know if such a thing exists, and if it does, I certainly don't
> have a copy.  Is anyone working on that?

I pushed a patch to remove the log_error messages from going to the
console by default.  The error logs still should be put in
/proc/ppc64/rtas/error_log for rtas_errd to pick them up and send it to
ELA for decode.  These apps are already done and can be found on IBM's
diagnostic service website.

http://techsupport.services.ibm.com/server/lopdiags/suselinux/pseries

Thanks,
Jake