[patch] Remove limit on MCA recoveries

[patch] Remove limit on MCA recoveries

[Russ Anderson]

The MCA recovery driver saves addresses memory errors
in an array.  The array has 32 entries.  The effect is 
that after 32 recoveries, the driver stops recovering.

This patch removes the page_isolate array.  Since the array
was only used to see if the page is already marked reserved,
check the reserved bit instead of the array.

Signed-off-by: Russ Anderson

-----------------------------------------
Index: 2.6.11/arch/ia64/kernel/mca_drv.c
=================================--- 2.6.11.orig/arch/ia64/kernel/mca_drv.c	2005-01-10 15:39:35.746124749 -0600
+++ 2.6.11/arch/ia64/kernel/mca_drv.c	2005-01-14 15:31:25.290081483 -0600
@@ -52,10 +52,6 @@
 	MCA_IS_GLOBAL = 1
 } mca_type_t;
 
-#define MAX_PAGE_ISOLATE 32
-
-static struct page *page_isolate[MAX_PAGE_ISOLATE];
-static int num_page_isolate = 0;
 
 typedef enum {
 	ISOLATE_NG = 0,
@@ -82,7 +78,6 @@
 static isolate_status_t
 mca_page_isolate(unsigned long paddr)
 {
-	int i;
 	struct page *p;
 
 	/* whether physical address is valid or not */
@@ -93,21 +88,15 @@
 	p = pfn_to_page(paddr>>PAGE_SHIFT);
 
 	/* check whether a page number have been already registered or not */
-	for( i = 0; i < num_page_isolate; i++ )
-		if( page_isolate[i] = p )
-			return ISOLATE_OK; /* already listed */
-
-	/* limitation check */
-	if( num_page_isolate = MAX_PAGE_ISOLATE ) 
-		return ISOLATE_NG;
+	if( PageReserved(p) )
+		return ISOLATE_OK; /* already listed */
 
 	/* kick pages having attribute 'SLAB' or 'Reserved' */
-	if( PageSlab(p) || PageReserved(p) ) 
+	if( PageSlab(p) ) 
 		return ISOLATE_NG;
 
 	/* add attribute 'Reserved' and register the page */
 	SetPageReserved(p);
-	page_isolate[num_page_isolate++] = p;
 
 	return ISOLATE_OK;
 }
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] Remove limit on MCA recoveries

[Keith Owens]

On Sat, 15 Jan 2005 16:49:06 -0600 (CST), 
Russ Anderson <rja@sgi.com> wrote:
>The MCA recovery driver saves addresses memory errors
>in an array.  The array has 32 entries.  The effect is 
>that after 32 recoveries, the driver stops recovering.
>
>This patch removes the page_isolate array.  Since the array
>was only used to see if the page is already marked reserved,
>check the reserved bit instead of the array.

lkcd dumps kernel pages marked reserved, so lkcd will try to process
isolated pages.  We will eventually need to add a new page flag to mark
faulty pages.

Re: [patch] Remove limit on MCA recoveries

[Matthias Fouquet-Lapar]

> On Sat, 15 Jan 2005 16:49:06 -0600 (CST), 
> Russ Anderson <rja@sgi.com> wrote:
> >The MCA recovery driver saves addresses memory errors
> >in an array.  The array has 32 entries.  The effect is 
> >that after 32 recoveries, the driver stops recovering.
> >
> >This patch removes the page_isolate array.  Since the array
> >was only used to see if the page is already marked reserved,
> >check the reserved bit instead of the array.
> 
> lkcd dumps kernel pages marked reserved, so lkcd will try to process
> isolated pages.  We will eventually need to add a new page flag to mark
> faulty pages.

Probably any other dump mechanism should be aware of bad HW pages as well,
so we might be better off to add a flag right away. While we are at it I
would propose to have actually two flags :

  - hard error (which will cause a MCA and should be skipped when taking
                a system dump)
  - soft error (page has encountered SBE, so we might want to avoid future
                allocation, but it can be dumped without causing an MCA)

Then we need to define some API that this information can be saved accross
a reboot, so we don't have to have another process hitting the UCE to find
out that there really is a problem at that location

Thanks

Matthias Fouquet-Lapar  Core Platform Software    mfl@sgi.com  VNET 521-8213
Principal Engineer      Silicon Graphics          Home Office (+33) 1 3047 4127

Re: [patch] Remove limit on MCA recoveries

[Hidetoshi Seto]

Russ Anderson wrote:
> The MCA recovery driver saves addresses memory errors
> in an array.  The array has 32 entries.  The effect is 
> that after 32 recoveries, the driver stops recovering.
> 
> This patch removes the page_isolate array.  Since the array
> was only used to see if the page is already marked reserved,
> check the reserved bit instead of the array.

This array is temporary designed for future use.
Similar to what Keith said, the array would be helpful if
something like dump application have no idea to avoid reading
MCA pages.

Roughly say, traditionally there are 2 type of pages:
  1 - not reserved
  2 - reserved
IMHO, there should be additional 3 type:
  3 - MCA reserved, not classified, don't read
  4 - Hard error (e.g. page on broken DIMM)
  5 - Soft error (e.g. having poisoned data)

The MCA recovery driver just does isolation.
What the driver want to do is marking 3 to the MCA pages.

It would better if type 3 pages could be classified into 4 or 5,
and more better if type 5 pages could be recycled into the system.

Thanks,
H.Seto

Re: [patch] Remove limit on MCA recoveries

[Russ Anderson]

Matthias Fouquet-Lapar wrote:
> Keith Owens wrote:
> > Russ Anderson <rja@sgi.com> wrote:
> > >The MCA recovery driver saves the addresses of memory errors
> > >in an array.  The array has 32 entries.  The effect is 
> > >that after 32 recoveries, the driver stops recovering.
> > >
> > >This patch removes the page_isolate array.  Since the array
> > >was only used to see if the page is already marked reserved,
> > >check the reserved bit instead of the array.
> > 
> > lkcd dumps kernel pages marked reserved, so lkcd will try to process
> > isolated pages.  We will eventually need to add a new page flag to mark
> > faulty pages.
> 
> Probably any other dump mechanism should be aware of bad HW pages as well,
> so we might be better off to add a flag right away.  While we are at it I
> would propose to have actually two flags :
> 
>   - hard error (which will cause a MCA and should be skipped when taking
>                 a system dump)
>   - soft error (page has encountered SBE, so we might want to avoid future
>                 allocation, but it can be dumped without causing an MCA)

Yes, there should be page->flags to indicate hard and soft memory errors,
such as PG_hard_error and PG_soft_error.  The dump code could look at those 
flags.

include/linux/page-flags.h has PG_error for indicating I/O errors, which 
is close but not quite what we need, given the way it is used.  

CCing linux-kernel since the flags are not ia64 specific.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] Remove limit on MCA recoveries

[Russ Anderson]

Hidetoshi Seto wrote:
> 
> This array is temporary designed for future use.
> Similar to what Keith said, the array would be helpful if
> something like dump application have no idea to avoid reading
> MCA pages.

I agree with that functionality, but think that using new
page->flags types would be a better implementation.

If page->flags are used to achieve that functionality, would
you object to removing the array?  Or is there additional data
that should be added to the array?

My only real complaint about the array is that the current size
is too small.  The Altix error injection test (which can modify
the ECC to create true memory uncorrectables) can recover from 
several hundred memory uncorrectable errors, when the array 
limitation is removed.  Making the array dynamic (linked list or 
something), so the size can be expanded would be a sufficient 
solution, too.
 
> Roughly say, traditionally there are 2 type of pages:
>   1 - not reserved
>   2 - reserved
> IMHO, there should be additional 3 type:
>   3 - MCA reserved, not classified, don't read
>   4 - Hard error (e.g. page on broken DIMM)
>   5 - Soft error (e.g. having poisoned data)
> 
> The MCA recovery driver just does isolation.
> What the driver want to do is marking 3 to the MCA pages.
> 
> It would better if type 3 pages could be classified into 4 or 5,

Yes.  Or just classify any that are not know to be 5 as 4 (assume
the worst classification unless sure it is a less sever classification).
That would remove the need for type 3.  

> and more better if type 5 pages could be recycled into the system.

Poison data can be cleaned up on Altix.  Those pages should not
be marked "reserved".  They should have the poison data cleaned up
and the pages reused, since the memory itself is not broken.  All
that is needed is the code to do it.  :-)

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] Remove limit on MCA recoveries

[Keith Owens]

On Mon, 17 Jan 2005 15:07:21 -0600 (CST), 
Russ Anderson <rja@sgi.com> wrote:
>Hidetoshi Seto wrote:
>My only real complaint about the array is that the current size
>is too small.  The Altix error injection test (which can modify
>the ECC to create true memory uncorrectables) can recover from 
>several hundred memory uncorrectable errors, when the array 
>limitation is removed.  Making the array dynamic (linked list or 
>something), so the size can be expanded would be a sufficient 
>solution, too.

Do not even think about making the array a dynamic size.  MCA events
can occur even when interrupts are disabled, which means that in an MCA
you have no idea if any of the kernel structures are in a safe state or
not.  That in turn means that you cannot extend the array from MCA
context.  Page flags that indicate bad memory are the best way to go.

RE: [patch] Remove limit on MCA recoveries

[Luck, Tony]

>Do not even think about making the array a dynamic size.  MCA events
>can occur even when interrupts are disabled, which means that in an MCA
>you have no idea if any of the kernel structures are in a safe state or
>not.  That in turn means that you cannot extend the array from MCA
>context.  Page flags that indicate bad memory are the best way to go.

Agreed.  If you want to increase the static limit up from 32 to a thousand
or so, as an interim fix until such extra page_flags are agreed upon, then
I'll be ok with that.

-Tony

Re: [patch] Remove limit on MCA recoveries

[Russ Anderson]

Keith Owens wrote:
> Russ Anderson <rja@sgi.com> wrote:
>
> >My only real complaint about the array is that the current size
> >is too small.  The Altix error injection test (which can modify
> >the ECC to create true memory uncorrectables) can recover from 
> >several hundred memory uncorrectable errors, when the array 
> >limitation is removed.  Making the array dynamic (linked list or 
> >something), so the size can be expanded would be a sufficient 
> >solution, too.
> 
> Do not even think about making the array a dynamic size.  MCA events
> can occur even when interrupts are disabled, which means that in an MCA
> you have no idea if any of the kernel structures are in a safe state or
> not.  That in turn means that you cannot extend the array from MCA
> context.  Page flags that indicate bad memory are the best way to go.

Good point Keith.

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] Remove limit on MCA recoveries

[Russ Anderson]

Tony Luck wrote:
> 
> Agreed.  If you want to increase the static limit up from 32 to a thousand
> or so, as an interim fix until such extra page_flags are agreed upon, then
> I'll be ok with that.

Sounds good.  Thanks, Tony.  Here is a patch.

Of course very quickly _only_ 1024 recoveries won't be enough.  :-)

Signed-off-by: Russ Anderson <rja@sgi.com>

----------------------------------------------------------------------------
Index: 2.6.11/arch/ia64/kernel/mca_drv.c
=================================--- 2.6.11.orig/arch/ia64/kernel/mca_drv.c	2005-01-17 15:55:26.280000969 -0600
+++ 2.6.11/arch/ia64/kernel/mca_drv.c	2005-01-17 15:55:56.151772456 -0600
@@ -52,7 +52,7 @@
 	MCA_IS_GLOBAL = 1
 } mca_type_t;
 
-#define MAX_PAGE_ISOLATE 32
+#define MAX_PAGE_ISOLATE 1024
 
 static struct page *page_isolate[MAX_PAGE_ISOLATE];
 static int num_page_isolate = 0;

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] Remove limit on MCA recoveries

[Matthias Fouquet-Lapar]

> > Agreed.  If you want to increase the static limit up from 32 to a thousand
> > or so, as an interim fix until such extra page_flags are agreed upon, then
> > I'll be ok with that.
> 
> Sounds good.  Thanks, Tony.  Here is a patch.
> 
> Of course very quickly _only_ 1024 recoveries won't be enough.  :-)

I think one option is a dynamic array, but any re-allocation would not be done
at MCA time. Once the table is full by 80% for example, we can increase it's size
at a somewhat more convenient time

Thanks

Matthias Fouquet-Lapar  Core Platform Software    mfl@sgi.com  VNET 521-8213
Principal Engineer      Silicon Graphics          Home Office (+33) 1 3047 4127