parisc DISCONTIGMEM compile breakage

parisc DISCONTIGMEM compile breakage

[Adrian Bunk]

Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
(mm: use two zonelist that are filtered by GFP mask)
causes the following build error with CONFIG_DISCONTIGMEM=y
on parisc:

<--  snip  -->

...
  CC      arch/parisc/mm/init.o
/home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c: In function 'show_mem':
/home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c:609: error: too few arguments to function 'node_zonelist'
make[2]: *** [arch/parisc/mm/init.o] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: parisc DISCONTIGMEM compile breakage

[Kyle McMartin]

On Tue, May 06, 2008 at 04:46:48PM +0300, Adrian Bunk wrote:
> Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> (mm: use two zonelist that are filtered by GFP mask)
> causes the following build error with CONFIG_DISCONTIGMEM=y
> on parisc:
> 
> <--  snip  -->
> 
> ...
>   CC      arch/parisc/mm/init.o
> /home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c: In function 'show_mem':
> /home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c:609: error: too few arguments to function 'node_zonelist'
> make[2]: *** [arch/parisc/mm/init.o] Error 1
> 

yes. the obvious fix works and boots, but I haven't quite determined why
they decided to add this yet, so no patch committed plz.

cheers, kyle

Re: parisc DISCONTIGMEM compile breakage

[Mel Gorman]

On (06/05/08 16:46), Adrian Bunk didst pronounce:
> Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> (mm: use two zonelist that are filtered by GFP mask)
> causes the following build error with CONFIG_DISCONTIGMEM=y
> on parisc:
> 

Thanks Adrian. I don't have a pa-risc cross-compiler and there isn't one
at the usual source http://userweb.kernel.org/~akpm/cross-compilers but the
patch below should fix it. Note that this debugging code should be deleted if
the mm-initialisation-and-debugging framework in -mm is merged to mainline
as it knows how to print all the zonelists where as the pa-risc equivilant
misses the GFP_THISNODE lists.

=====
Subject: [PATCH] Fix pa-risc DISCONTIGMEM compile breakage
    
PA-RISC to aid debugging prints out the zonelists setup by the system. A
bad call to node_zonelist() breaks at compile-time. This patch fixes it.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
---
 arch/parisc/mm/init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
index 1f01284..b0ed709 100644
--- a/arch/parisc/mm/init.c
+++ b/arch/parisc/mm/init.c
@@ -606,7 +606,7 @@ void show_mem(void)
 		int i, j;
 
 		for (i = 0; i < npmem_ranges; i++) {
-			zl = node_zonelist(i);
+			zl = node_zonelist(i, 0);
 			for (j = 0; j < MAX_NR_ZONES; j++) {
 				struct zoneref *z;
 				struct zone *zone;

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: parisc DISCONTIGMEM compile breakage

[Mel Gorman]

On (06/05/08 11:24), Kyle McMartin didst pronounce:
> On Tue, May 06, 2008 at 04:46:48PM +0300, Adrian Bunk wrote:
> > Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> > (mm: use two zonelist that are filtered by GFP mask)
> > causes the following build error with CONFIG_DISCONTIGMEM=y
> > on parisc:
> > 
> > <--  snip  -->
> > 
> > ...
> >   CC      arch/parisc/mm/init.o
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c: In function 'show_mem':
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/arch/parisc/mm/init.c:609: error: too few arguments to function 'node_zonelist'
> > make[2]: *** [arch/parisc/mm/init.o] Error 1
> > 
> 
> yes. the obvious fix works and boots, but I haven't quite determined why
> they decided to add this yet, so no patch committed plz.
> 

I'm not sure what you are referring to as "this" here. If you mean the
zonelist changes, they were done to reduce the number of zonelists that
exist, to clear up an anomoly with MPOL_BIND and to remove a hack called
alloc_should_filter_zonelist() in the page allocator. The pa-risc code that
walks zonelists (presumably for debugging) has been around for a while and
was updated at the same time as the zonelist patches were done, just not as
well as it should have been.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: parisc DISCONTIGMEM compile breakage

[Christoph Lameter]

Reviewed-by: Christoph Lameter <clameter@sgi.com>

Re: parisc DISCONTIGMEM compile breakage

[James Bottomley]

On Tue, 2008-05-06 at 16:27 +0100, Mel Gorman wrote:
> On (06/05/08 16:46), Adrian Bunk didst pronounce:
> > Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> > (mm: use two zonelist that are filtered by GFP mask)
> > causes the following build error with CONFIG_DISCONTIGMEM=y
> > on parisc:
> > 
> 
> Thanks Adrian. I don't have a pa-risc cross-compiler and there isn't one
> at the usual source http://userweb.kernel.org/~akpm/cross-compilers but the
> patch below should fix it. Note that this debugging code should be deleted if
> the mm-initialisation-and-debugging framework in -mm is merged to mainline
> as it knows how to print all the zonelists where as the pa-risc equivilant
> misses the GFP_THISNODE lists.
> 
> =====
> Subject: [PATCH] Fix pa-risc DISCONTIGMEM compile breakage
>     
> PA-RISC to aid debugging prints out the zonelists setup by the system. A
> bad call to node_zonelist() breaks at compile-time. This patch fixes it.
> 
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
>  arch/parisc/mm/init.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
> index 1f01284..b0ed709 100644
> --- a/arch/parisc/mm/init.c
> +++ b/arch/parisc/mm/init.c
> @@ -606,7 +606,7 @@ void show_mem(void)
>  		int i, j;
>  
>  		for (i = 0; i < npmem_ranges; i++) {
> -			zl = node_zonelist(i);
> +			zl = node_zonelist(i, 0);
>  			for (j = 0; j < MAX_NR_ZONES; j++) {

This rather doesn't look right.  The intent is for i to iterate over the
ranges (as nodes) and j to iterate over the zones to get the zonelist.
The original variable k was going over the mappings, which you replaced
with for_each_zone_zonelist.  If you use node_zonelist(i, 0) you don't
really look at the actual zones in the map.

Therefore it seems what will give the original functionality back is
actually this:

James

diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
index 1f01284..7bc286d 100644
--- a/arch/parisc/mm/init.c
+++ b/arch/parisc/mm/init.c
@@ -602,15 +602,16 @@ void show_mem(void)
 
 #ifdef CONFIG_DISCONTIGMEM
 	{
-		struct zonelist *zl;
 		int i, j;
 
 		for (i = 0; i < npmem_ranges; i++) {
-			zl = node_zonelist(i);
 			for (j = 0; j < MAX_NR_ZONES; j++) {
+				struct zonelist *zl;
 				struct zoneref *z;
 				struct zone *zone;
 
+				zl = NODE_DATA(i)->node_zonelists + j;
+
 				printk("Zone list for zone %d on node %d: ", j, i);
 				for_each_zone_zonelist(zone, z, zl, j)
 					printk("[%d/%s] ", zone_to_nid(zone),

Re: parisc DISCONTIGMEM compile breakage

[Mel Gorman]

On (06/05/08 14:58), James Bottomley didst pronounce:
> On Tue, 2008-05-06 at 16:27 +0100, Mel Gorman wrote:
> > On (06/05/08 16:46), Adrian Bunk didst pronounce:
> > > Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> > > (mm: use two zonelist that are filtered by GFP mask)
> > > causes the following build error with CONFIG_DISCONTIGMEM=y
> > > on parisc:
> > > 
> > 
> > Thanks Adrian. I don't have a pa-risc cross-compiler and there isn't one
> > at the usual source http://userweb.kernel.org/~akpm/cross-compilers but the
> > patch below should fix it. Note that this debugging code should be deleted if
> > the mm-initialisation-and-debugging framework in -mm is merged to mainline
> > as it knows how to print all the zonelists where as the pa-risc equivilant
> > misses the GFP_THISNODE lists.
> > 
> > =====
> > Subject: [PATCH] Fix pa-risc DISCONTIGMEM compile breakage
> >     
> > PA-RISC to aid debugging prints out the zonelists setup by the system. A
> > bad call to node_zonelist() breaks at compile-time. This patch fixes it.
> > 
> > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> > ---
> >  arch/parisc/mm/init.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
> > index 1f01284..b0ed709 100644
> > --- a/arch/parisc/mm/init.c
> > +++ b/arch/parisc/mm/init.c
> > @@ -606,7 +606,7 @@ void show_mem(void)
> >  		int i, j;
> >  
> >  		for (i = 0; i < npmem_ranges; i++) {
> > -			zl = node_zonelist(i);
> > +			zl = node_zonelist(i, 0);
> >  			for (j = 0; j < MAX_NR_ZONES; j++) {
> 
> This rather doesn't look right.  The intent is for i to iterate over the
> ranges (as nodes) and j to iterate over the zones to get the zonelist.

Yes, that appears to be the original intention.

> The original variable k was going over the mappings, which you replaced
> with for_each_zone_zonelist.  If you use node_zonelist(i, 0) you don't
> really look at the actual zones in the map.
> 

No, but you filter the one list based on the highest zone that can be
accesssed.

> Therefore it seems what will give the original functionality back is
> actually this:
> 

To be fair, a few weeks ago you would have been right. Since then zonelists
have changed though so that there are only two zonelists per node in
git-latest. One which is for general allocations and one for GFP_THISNODE. The
zonelist is filtered based on the GFP flags, hence the iterator. I copied the
parisc code with the compile-fix into mm/page_alloc.c and called it just to
be sure and with qemu, I see

[    0.628960] Zone list for zone 0 on node 0: [0/DMA] 
[    0.628960] Zone list for zone 1 on node 0: [0/Normal] [0/DMA] 
[    0.628960] Zone list for zone 2 on node 0: [0/HighMem] [0/Normal] [0/DMA] 
[    0.628960] Zone list for zone 3 on node 0: [0/Movable] [0/HighMem] [0/Normal] [0/DMA]

which looks right (movablecore was specified hence the existance of the
movable zone).

> <SNIP>
>  			for (j = 0; j < MAX_NR_ZONES; j++) {
> +				struct zonelist *zl;
>  				struct zoneref *z;
>  				struct zone *zone;
>  
> +				zl = NODE_DATA(i)->node_zonelists + j;
> +

This would read past the end of the node_zonelists array which is only
MAX_ZONELISTS (2 for CONFIG_NUMA) in size.

>  				printk("Zone list for zone %d on node %d: ", j, i);
>  				for_each_zone_zonelist(zone, z, zl, j)
>  					printk("[%d/%s] ", zone_to_nid(zone),
> 
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: parisc DISCONTIGMEM compile breakage

[James Bottomley]

On Tue, 2008-05-06 at 22:17 +0100, Mel Gorman wrote:
> On (06/05/08 14:58), James Bottomley didst pronounce:
> > On Tue, 2008-05-06 at 16:27 +0100, Mel Gorman wrote:
> > > On (06/05/08 16:46), Adrian Bunk didst pronounce:
> > > > Commit 54a6eb5c4765aa573a030ceeba2c14e3d2ea5706
> > > > (mm: use two zonelist that are filtered by GFP mask)
> > > > causes the following build error with CONFIG_DISCONTIGMEM=y
> > > > on parisc:
> > > > 
> > > 
> > > Thanks Adrian. I don't have a pa-risc cross-compiler and there isn't one
> > > at the usual source http://userweb.kernel.org/~akpm/cross-compilers but the
> > > patch below should fix it. Note that this debugging code should be deleted if
> > > the mm-initialisation-and-debugging framework in -mm is merged to mainline
> > > as it knows how to print all the zonelists where as the pa-risc equivilant
> > > misses the GFP_THISNODE lists.
> > > 
> > > =====
> > > Subject: [PATCH] Fix pa-risc DISCONTIGMEM compile breakage
> > >     
> > > PA-RISC to aid debugging prints out the zonelists setup by the system. A
> > > bad call to node_zonelist() breaks at compile-time. This patch fixes it.
> > > 
> > > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> > > ---
> > >  arch/parisc/mm/init.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
> > > index 1f01284..b0ed709 100644
> > > --- a/arch/parisc/mm/init.c
> > > +++ b/arch/parisc/mm/init.c
> > > @@ -606,7 +606,7 @@ void show_mem(void)
> > >  		int i, j;
> > >  
> > >  		for (i = 0; i < npmem_ranges; i++) {
> > > -			zl = node_zonelist(i);
> > > +			zl = node_zonelist(i, 0);
> > >  			for (j = 0; j < MAX_NR_ZONES; j++) {
> > 
> > This rather doesn't look right.  The intent is for i to iterate over the
> > ranges (as nodes) and j to iterate over the zones to get the zonelist.
> 
> Yes, that appears to be the original intention.
> 
> > The original variable k was going over the mappings, which you replaced
> > with for_each_zone_zonelist.  If you use node_zonelist(i, 0) you don't
> > really look at the actual zones in the map.
> > 
> 
> No, but you filter the one list based on the highest zone that can be
> accesssed.
> 
> > Therefore it seems what will give the original functionality back is
> > actually this:
> > 
> 
> To be fair, a few weeks ago you would have been right. Since then zonelists
> have changed though so that there are only two zonelists per node in
> git-latest. One which is for general allocations and one for GFP_THISNODE. The
> zonelist is filtered based on the GFP flags, hence the iterator. I copied the
> parisc code with the compile-fix into mm/page_alloc.c and called it just to
> be sure and with qemu, I see
> 
> [    0.628960] Zone list for zone 0 on node 0: [0/DMA] 
> [    0.628960] Zone list for zone 1 on node 0: [0/Normal] [0/DMA] 
> [    0.628960] Zone list for zone 2 on node 0: [0/HighMem] [0/Normal] [0/DMA] 
> [    0.628960] Zone list for zone 3 on node 0: [0/Movable] [0/HighMem] [0/Normal] [0/DMA]
> 
> which looks right (movablecore was specified hence the existance of the
> movable zone).

Thanks ... I confirmed that your fix does actually work.  Mysteriously,
this is what it produces as output:

Zone list for zone 0 on node 0: [0/Normal] [0/Normal] 
Zone list for zone 1 on node 0: [0/Normal] [0/Normal] 
Zone list for zone 0 on node 1: [0/Normal] [0/Normal] 
Zone list for zone 1 on node 1: [0/Normal] [0/Normal] 

Which is correct

James