Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-19 Thread Dave Hansen 
On 10/18/2012 03:20 PM, Andrew Morton wrote:
> On Wed, 17 Oct 2012 08:09:55 -0700
> Dave Hansen  wrote:
>>> +#ifdef CONFIG_MEMORY_FAILURE
>>> +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
>>> +{
>>> +   int i;
>>> +
>>> +   if (!memmap)
>>> +   return;
>>
>> I guess free_section_usemap() does the same thing.
> 
> What does this observation mean?

sparse_remove_one_section() has an if(ms->section_mem_map) statement.
Inside that if() block is the only place in the function where 'memmap'
can get set.

Currently, sparse_remove_one_section() calls in to free_section_usemap()
ouside of that if() block.  With this patch new call to
clear_hwpoisoned_pages() is done in the same place, both passing 'memmap'.

However, both free_section_usemap() and clear_hwpoisoned_pages() check
'memmap' for NULL and immediately return if so.  That's a bit silly
since it could hide garbage coming back from sparse_decode_mem_map().
Seems like we should just call them both inside that if() block, or
reorganize sparse_remove_one_section(), maybe like this:

void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
{
struct page *memmap = NULL;
unsigned long *usemap = NULL;

if (!ms->section_mem_map)
return;

usemap = ms->pageblock_flags;
memmap = sparse_decode_mem_map(ms->section_mem_map,
__section_nr(ms));
ms->section_mem_map = 0;
ms->pageblock_flags = NULL;

free_section_usemap(memmap, usemap);
clear_hwpoisoned_pages(usemap, ...);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-19 Thread Dave Hansen 
On 10/18/2012 03:20 PM, Andrew Morton wrote:
 On Wed, 17 Oct 2012 08:09:55 -0700
 Dave Hansen d...@linux.vnet.ibm.com wrote:
 +#ifdef CONFIG_MEMORY_FAILURE
 +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
 +{
 +   int i;
 +
 +   if (!memmap)
 +   return;

 I guess free_section_usemap() does the same thing.
 
 What does this observation mean?

sparse_remove_one_section() has an if(ms-section_mem_map) statement.
Inside that if() block is the only place in the function where 'memmap'
can get set.

Currently, sparse_remove_one_section() calls in to free_section_usemap()
ouside of that if() block.  With this patch new call to
clear_hwpoisoned_pages() is done in the same place, both passing 'memmap'.

However, both free_section_usemap() and clear_hwpoisoned_pages() check
'memmap' for NULL and immediately return if so.  That's a bit silly
since it could hide garbage coming back from sparse_decode_mem_map().
Seems like we should just call them both inside that if() block, or
reorganize sparse_remove_one_section(), maybe like this:

void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
{
struct page *memmap = NULL;
unsigned long *usemap = NULL;

if (!ms-section_mem_map)
return;

usemap = ms-pageblock_flags;
memmap = sparse_decode_mem_map(ms-section_mem_map,
__section_nr(ms));
ms-section_mem_map = 0;
ms-pageblock_flags = NULL;

free_section_usemap(memmap, usemap);
clear_hwpoisoned_pages(usemap, ...);
}

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-18 Thread Andrew Morton 
On Wed, 17 Oct 2012 08:09:55 -0700
Dave Hansen  wrote:

> Hi Wen,
> 
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> > +{
> > +   int i;
> > +
> > +   if (!memmap)
> > +   return;
> 
> I guess free_section_usemap() does the same thing.

What does this observation mean?

> > +   for (i = 0; i < PAGES_PER_SECTION; i++) {
> > +   if (PageHWPoison([i])) {
> > +   atomic_long_sub(1, _bad_pages);
> > +   ClearPageHWPoison([i]);
> > +   }
> > +   }
> > +}
> > +#endif
> > +
> >  void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
> >  {
> > struct page *memmap = NULL;
>
> ..
>
> and keep the #ifdef out of sparse_remove_one_section().

yup.

--- 
a/mm/sparse.c~memory-hotplug-update-mce_bad_pages-when-removing-the-memory-fix
+++ a/mm/sparse.c
@@ -788,6 +788,10 @@ static void clear_hwpoisoned_pages(struc
}
}
 }
+#else
+static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+}
 #endif
 
 void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
@@ -803,10 +807,7 @@ void sparse_remove_one_section(struct zo
ms->pageblock_flags = NULL;
}
 
-#ifdef CONFIG_MEMORY_FAILURE
clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
-#endif
-
free_section_usemap(memmap, usemap);
 }
 #endif
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-18 Thread Andrew Morton 
On Wed, 17 Oct 2012 08:09:55 -0700
Dave Hansen d...@linux.vnet.ibm.com wrote:

 Hi Wen,
 
  +#ifdef CONFIG_MEMORY_FAILURE
  +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
  +{
  +   int i;
  +
  +   if (!memmap)
  +   return;
 
 I guess free_section_usemap() does the same thing.

What does this observation mean?

  +   for (i = 0; i  PAGES_PER_SECTION; i++) {
  +   if (PageHWPoison(memmap[i])) {
  +   atomic_long_sub(1, mce_bad_pages);
  +   ClearPageHWPoison(memmap[i]);
  +   }
  +   }
  +}
  +#endif
  +
   void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
   {
  struct page *memmap = NULL;

 ..

 and keep the #ifdef out of sparse_remove_one_section().

yup.

--- 
a/mm/sparse.c~memory-hotplug-update-mce_bad_pages-when-removing-the-memory-fix
+++ a/mm/sparse.c
@@ -788,6 +788,10 @@ static void clear_hwpoisoned_pages(struc
}
}
 }
+#else
+static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+}
 #endif
 
 void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
@@ -803,10 +807,7 @@ void sparse_remove_one_section(struct zo
ms-pageblock_flags = NULL;
}
 
-#ifdef CONFIG_MEMORY_FAILURE
clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
-#endif
-
free_section_usemap(memmap, usemap);
 }
 #endif
_

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-17 Thread Dave Hansen 
Hi Wen,

> +#ifdef CONFIG_MEMORY_FAILURE
> +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> +{
> + int i;
> +
> + if (!memmap)
> + return;

I guess free_section_usemap() does the same thing.

> + for (i = 0; i < PAGES_PER_SECTION; i++) {
> + if (PageHWPoison([i])) {
> + atomic_long_sub(1, _bad_pages);
> + ClearPageHWPoison([i]);
> + }
> + }
> +}
> +#endif
> +
>  void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
>  {
>   struct page *memmap = NULL;
> @@ -786,6 +803,10 @@ void sparse_remove_one_section(struct zone *zone, struct 
> mem_section *ms)
>   ms->pageblock_flags = NULL;
>   }
> 
> +#ifdef CONFIG_MEMORY_FAILURE
> + clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
> +#endif
> +
>   free_section_usemap(memmap, usemap);
>  }
>  #endif

But why put the call outside the  "if (ms->section_mem_map)" block?  If
you put it inside, then you don't have to check for !memmap in
clear_hwpoisoned_pages().

Also, we really frown on #ifdefs scattered throughout code.  I'd suggest
either:

+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+#ifdef CONFIG_MEMORY_FAILURE
... existing code
+#endif /* CONFIG_MEMORY_FAILURE */
+}

or

+#ifdef CONFIG_MEMORY_FAILURE
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
... existing code
+}
+#else
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{}
+#endif /* CONFIG_MEMORY_FAILURE */

and keep the #ifdef out of sparse_remove_one_section().

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-17 Thread wency 
From: Wen Congyang 

When we hotremove a memory device, we will free the memory to store
struct page. If the page is hwpoisoned page, we should decrease
mce_bad_pages.

CC: David Rientjes 
CC: Jiang Liu 
CC: Len Brown 
CC: Benjamin Herrenschmidt 
CC: Paul Mackerras 
CC: Christoph Lameter 
Cc: Minchan Kim 
CC: Andrew Morton 
CC: KOSAKI Motohiro 
CC: Yasuaki Ishimatsu 
Signed-off-by: Wen Congyang 
---
 mm/sparse.c |   21 +
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index fac95f2..24072e4 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -773,6 +773,23 @@ out:
return ret;
 }
 
+#ifdef CONFIG_MEMORY_FAILURE
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+   int i;
+
+   if (!memmap)
+   return;
+
+   for (i = 0; i < PAGES_PER_SECTION; i++) {
+   if (PageHWPoison([i])) {
+   atomic_long_sub(1, _bad_pages);
+   ClearPageHWPoison([i]);
+   }
+   }
+}
+#endif
+
 void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
 {
struct page *memmap = NULL;
@@ -786,6 +803,10 @@ void sparse_remove_one_section(struct zone *zone, struct 
mem_section *ms)
ms->pageblock_flags = NULL;
}
 
+#ifdef CONFIG_MEMORY_FAILURE
+   clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
+#endif
+
free_section_usemap(memmap, usemap);
 }
 #endif
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-17 Thread wency 
From: Wen Congyang we...@cn.fujitsu.com

When we hotremove a memory device, we will free the memory to store
struct page. If the page is hwpoisoned page, we should decrease
mce_bad_pages.

CC: David Rientjes rient...@google.com
CC: Jiang Liu liu...@gmail.com
CC: Len Brown len.br...@intel.com
CC: Benjamin Herrenschmidt b...@kernel.crashing.org
CC: Paul Mackerras pau...@samba.org
CC: Christoph Lameter c...@linux.com
Cc: Minchan Kim minchan@gmail.com
CC: Andrew Morton a...@linux-foundation.org
CC: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com
CC: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
Signed-off-by: Wen Congyang we...@cn.fujitsu.com
---
 mm/sparse.c |   21 +
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index fac95f2..24072e4 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -773,6 +773,23 @@ out:
return ret;
 }
 
+#ifdef CONFIG_MEMORY_FAILURE
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+   int i;
+
+   if (!memmap)
+   return;
+
+   for (i = 0; i  PAGES_PER_SECTION; i++) {
+   if (PageHWPoison(memmap[i])) {
+   atomic_long_sub(1, mce_bad_pages);
+   ClearPageHWPoison(memmap[i]);
+   }
+   }
+}
+#endif
+
 void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
 {
struct page *memmap = NULL;
@@ -786,6 +803,10 @@ void sparse_remove_one_section(struct zone *zone, struct 
mem_section *ms)
ms-pageblock_flags = NULL;
}
 
+#ifdef CONFIG_MEMORY_FAILURE
+   clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
+#endif
+
free_section_usemap(memmap, usemap);
 }
 #endif
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/5] memory-hotplug: update mce_bad_pages when removing the memory

2012-10-17 Thread Dave Hansen 
Hi Wen,

 +#ifdef CONFIG_MEMORY_FAILURE
 +static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
 +{
 + int i;
 +
 + if (!memmap)
 + return;

I guess free_section_usemap() does the same thing.

 + for (i = 0; i  PAGES_PER_SECTION; i++) {
 + if (PageHWPoison(memmap[i])) {
 + atomic_long_sub(1, mce_bad_pages);
 + ClearPageHWPoison(memmap[i]);
 + }
 + }
 +}
 +#endif
 +
  void sparse_remove_one_section(struct zone *zone, struct mem_section *ms)
  {
   struct page *memmap = NULL;
 @@ -786,6 +803,10 @@ void sparse_remove_one_section(struct zone *zone, struct 
 mem_section *ms)
   ms-pageblock_flags = NULL;
   }
 
 +#ifdef CONFIG_MEMORY_FAILURE
 + clear_hwpoisoned_pages(memmap, PAGES_PER_SECTION);
 +#endif
 +
   free_section_usemap(memmap, usemap);
  }
  #endif

But why put the call outside the  if (ms-section_mem_map) block?  If
you put it inside, then you don't have to check for !memmap in
clear_hwpoisoned_pages().

Also, we really frown on #ifdefs scattered throughout code.  I'd suggest
either:

+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
+#ifdef CONFIG_MEMORY_FAILURE
... existing code
+#endif /* CONFIG_MEMORY_FAILURE */
+}

or

+#ifdef CONFIG_MEMORY_FAILURE
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{
... existing code
+}
+#else
+static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+{}
+#endif /* CONFIG_MEMORY_FAILURE */

and keep the #ifdef out of sparse_remove_one_section().

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/