* Re: [patch] MCA recovery verify pfn_valid
2005-09-16 21:54 [patch] MCA recovery verify pfn_valid Russ Anderson
@ 2005-09-19 22:19 ` Russ Anderson
2005-09-20 7:34 ` Hidetoshi Seto
2005-09-20 22:07 ` Russ Anderson
2 siblings, 0 replies; 4+ messages in thread
From: Russ Anderson @ 2005-09-19 22:19 UTC (permalink / raw)
To: linux-ia64
Jack Steiner wrote:
> > On Fri, Sep 16, 2005 at 04:54:53PM -0500, Russ Anderson wrote:
> >
> > Verify the pfn is valid before calling pfn_to_page().
> >
> > Signed-off-by: Russ Anderson (rja@sgi.com)
> >
> > ---------------------------------------------------------------------
> > arch/ia64/kernel/mca_drv.c | 3 +++
> > 1 files changed, 3 insertions(+)
> >
> > Index: linux-2.6.git#test/arch/ia64/kernel/mca_drv.c
> > =================================> > --- linux-2.6.git#test.orig/arch/ia64/kernel/mca_drv.c 2005-09-16 16:28:11.857773257 -0500
> > +++ linux-2.6.git#test/arch/ia64/kernel/mca_drv.c 2005-09-16 16:29:32.119181669 -0500
> > @@ -87,6 +87,9 @@ mca_page_isolate(unsigned long paddr)
> > if ( !ia64_phys_addr_valid(paddr) )
> > return ISOLATE_NG;
> >
> > + if ( !pfn_valid(paddr))
> > + return ISOLATE_OK;
>
> Should this be ISOLATE_OK or ISOLATE_NG?
>
> If the page is from another partition, the page has not been isolated? I realize
> that the return status is not used for anything but printing a message, but it
> seems misleading to print that isolation was successful when nothing was actually done.
>
> Perhaps another status should be returned.
In this case, is any message really needed? As you say, success is
is misleading because nothing was done. Failure is not accurate
because it didn't fail. So perhaps a third status that does not
print any page isolation message is the best solution.
For reference, the code:
----------------------------------------------------------
void
mca_handler_bh(unsigned long paddr)
{
printk(KERN_DEBUG "OS_MCA: process [pid: %d](%s) encounters MCA.\n",
current->pid, current->comm);
spin_lock(&mca_bh_lock);
if (mca_page_isolate(paddr) = ISOLATE_OK) {
printk(KERN_DEBUG "Page isolation: ( %lx ) success.\n", paddr);
} else {
printk(KERN_DEBUG "Page isolation: ( %lx ) failure.\n", paddr);
}
spin_unlock(&mca_bh_lock);
/* This process is about to be killed itself */
do_exit(SIGKILL);
}
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] MCA recovery verify pfn_valid
2005-09-16 21:54 [patch] MCA recovery verify pfn_valid Russ Anderson
2005-09-19 22:19 ` Russ Anderson
@ 2005-09-20 7:34 ` Hidetoshi Seto
2005-09-20 22:07 ` Russ Anderson
2 siblings, 0 replies; 4+ messages in thread
From: Hidetoshi Seto @ 2005-09-20 7:34 UTC (permalink / raw)
To: linux-ia64
[-- Attachment #1: Type: text/plain, Size: 553 bytes --]
How about this?
Thanks,
H.Seto
Verify the pfn is valid before calling pfn_to_page(),
and cut isolation message if nothing was done.
Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Russ Anderson wrote:
> Jack Steiner wrote:
>>
>>Perhaps another status should be returned.
>
> In this case, is any message really needed? As you say, success is
> is misleading because nothing was done. Failure is not accurate
> because it didn't fail. So perhaps a third status that does not
> print any page isolation message is the best solution.
[-- Attachment #2: mca_drv_pfnverify.patch --]
[-- Type: text/plain, Size: 1515 bytes --]
arch/ia64/kernel/mca_drv.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
Index: build/arch/ia64/kernel/mca_drv.c
===================================================================
--- build.orig/arch/ia64/kernel/mca_drv.c
+++ build/arch/ia64/kernel/mca_drv.c
@@ -56,8 +56,9 @@ static struct page *page_isolate[MAX_PAG
static int num_page_isolate = 0;
typedef enum {
- ISOLATE_NG = 0,
- ISOLATE_OK = 1
+ ISOLATE_NG,
+ ISOLATE_OK,
+ ISOLATE_NONE
} isolate_status_t;
/*
@@ -74,7 +75,7 @@ static struct {
* @paddr: poisoned memory location
*
* Return value:
- * ISOLATE_OK / ISOLATE_NG
+ * one of isolate_status_t, ISOLATE_OK/NG/NONE.
*/
static isolate_status_t
@@ -85,7 +86,10 @@ mca_page_isolate(unsigned long paddr)
/* whether physical address is valid or not */
if (!ia64_phys_addr_valid(paddr))
- return ISOLATE_NG;
+ return ISOLATE_NONE;
+
+ if (!pfn_valid(paddr))
+ return ISOLATE_NONE;
/* convert physical address to physical page number */
p = pfn_to_page(paddr>>PAGE_SHIFT);
@@ -122,10 +126,15 @@ mca_handler_bh(unsigned long paddr)
current->pid, current->comm);
spin_lock(&mca_bh_lock);
- if (mca_page_isolate(paddr) == ISOLATE_OK) {
+ switch (mca_page_isolate(paddr)) {
+ case ISOLATE_OK:
printk(KERN_DEBUG "Page isolation: ( %lx ) success.\n", paddr);
- } else {
+ break;
+ case ISOLATE_NG:
printk(KERN_DEBUG "Page isolation: ( %lx ) failure.\n", paddr);
+ break;
+ default:
+ break;
}
spin_unlock(&mca_bh_lock);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] MCA recovery verify pfn_valid
2005-09-16 21:54 [patch] MCA recovery verify pfn_valid Russ Anderson
2005-09-19 22:19 ` Russ Anderson
2005-09-20 7:34 ` Hidetoshi Seto
@ 2005-09-20 22:07 ` Russ Anderson
2 siblings, 0 replies; 4+ messages in thread
From: Russ Anderson @ 2005-09-20 22:07 UTC (permalink / raw)
To: linux-ia64
Hidetoshi Seto wrote:
>
> How about this?
>
> Thanks,
> H.Seto
>
> Verify the pfn is valid before calling pfn_to_page(),
> and cut isolation message if nothing was done.
>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Looks good to me.
--
Russ Anderson, OS RAS/Partitioning Project Lead
SGI - Silicon Graphics Inc rja@sgi.com
^ permalink raw reply [flat|nested] 4+ messages in thread