2012-06-29 20:15:03

by Rabin Vincent

[permalink] [raw]
Subject: [PATCH] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

0ee332c14518699 ("memblock: Kill early_node_map[]") wanted to replace
CONFIG_ARCH_POPULATES_NODE_MAP with CONFIG_HAVE_MEMBLOCK_NODE_MAP but
ended up replacing one occurence with a reference to the non-existent
symbol CONFIG_HAVE_MEMBLOCK_NODE.

Signed-off-by: Rabin Vincent <[email protected]>
---
include/linux/mmzone.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 2427706..1d0a9a3 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -773,7 +773,7 @@ extern int movable_zone;

static inline int zone_movable_is_highmem(void)
{
-#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE)
+#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
return movable_zone == ZONE_HIGHMEM;
#else
return 0;
--
1.7.9.5


2012-06-29 20:21:44

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

On Sat, Jun 30, 2012 at 01:44:37AM +0530, Rabin Vincent wrote:
> 0ee332c14518699 ("memblock: Kill early_node_map[]") wanted to replace
> CONFIG_ARCH_POPULATES_NODE_MAP with CONFIG_HAVE_MEMBLOCK_NODE_MAP but
> ended up replacing one occurence with a reference to the non-existent
> symbol CONFIG_HAVE_MEMBLOCK_NODE.
>
> Signed-off-by: Rabin Vincent <[email protected]>

Please add Cc: [email protected].

Acked-by: Tejun Heo <[email protected]>

> ---
> include/linux/mmzone.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 2427706..1d0a9a3 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -773,7 +773,7 @@ extern int movable_zone;
>
> static inline int zone_movable_is_highmem(void)
> {
> -#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE)
> +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> return movable_zone == ZONE_HIGHMEM;

This probably has been doing something weird to 32bit machines w/
memory hotplug configured (which probably are extremely rare). It
would be nice if the patch description explained what behavior it
actually fixes. How did you find this? From config check util or did
you actually see something broken?

Thanks.

--
tejun

2012-06-29 20:22:00

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

On Sat, 30 Jun 2012 01:44:37 +0530
Rabin Vincent <[email protected]> wrote:

> 0ee332c14518699 ("memblock: Kill early_node_map[]") wanted to replace
> CONFIG_ARCH_POPULATES_NODE_MAP with CONFIG_HAVE_MEMBLOCK_NODE_MAP but
> ended up replacing one occurence with a reference to the non-existent
> symbol CONFIG_HAVE_MEMBLOCK_NODE.
>
> Signed-off-by: Rabin Vincent <[email protected]>
> ---
> include/linux/mmzone.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 2427706..1d0a9a3 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -773,7 +773,7 @@ extern int movable_zone;
>
> static inline int zone_movable_is_highmem(void)
> {
> -#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE)
> +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> return movable_zone == ZONE_HIGHMEM;
> #else
> return 0;

Huh. I wonder why nobody noticed.

2012-06-29 20:23:39

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

On Fri, Jun 29, 2012 at 01:21:57PM -0700, Andrew Morton wrote:
> > static inline int zone_movable_is_highmem(void)
> > {
> > -#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE)
> > +#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> > return movable_zone == ZONE_HIGHMEM;
> > #else
> > return 0;
>
> Huh. I wonder why nobody noticed.

Well, the combination is 32bit + memory hotplug....

--
tejun

2012-06-29 20:59:16

by Rabin Vincent

[permalink] [raw]
Subject: Re: [PATCH] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

On Fri, Jun 29, 2012 at 01:21:39PM -0700, Tejun Heo wrote:
> This probably has been doing something weird to 32bit machines w/
> memory hotplug configured (which probably are extremely rare). It
> would be nice if the patch description explained what behavior it
> actually fixes. How did you find this? From config check util or did
> you actually see something broken?

Neither, I just noticed it while debugging some unrelated problems. The
arch I work on (ARM) doesn't even select this so I don't have any
observed behaviour to describe -- I'll just add a paraphrase of your
statement about who's affected to the patch description.

8<--------------------
>From 38c7a50331efa83e4d07cbd1d6364d62e4448482 Mon Sep 17 00:00:00 2001
From: Rabin Vincent <[email protected]>
Date: Sat, 30 Jun 2012 01:17:45 +0530
Subject: [PATCHv2] mm: CONFIG_HAVE_MEMBLOCK_NODE -> CONFIG_HAVE_MEMBLOCK_NODE_MAP

0ee332c14518699 ("memblock: Kill early_node_map[]") wanted to replace
CONFIG_ARCH_POPULATES_NODE_MAP with CONFIG_HAVE_MEMBLOCK_NODE_MAP but
ended up replacing one occurence with a reference to the non-existent
symbol CONFIG_HAVE_MEMBLOCK_NODE.

The resulting omission of code would probably have been causing problems
to 32-bit machines with memory hotplug.

Cc: [email protected]
Acked-by: Tejun Heo <[email protected]>
Signed-off-by: Rabin Vincent <[email protected]>
---
include/linux/mmzone.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 2427706..1d0a9a3 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -773,7 +773,7 @@ extern int movable_zone;

static inline int zone_movable_is_highmem(void)
{
-#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE)
+#if defined(CONFIG_HIGHMEM) && defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
return movable_zone == ZONE_HIGHMEM;
#else
return 0;
--
1.7.9.5