[PATCH] Simplify node/zone field in page->flags

[PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

Currently we keep track of a pages node & zone in the top 8 bits (on 
32-bit arches, 10 bits on 64-bit arches) of page->flags.  We typically 
compute the field as follows:
	node_num * MAX_NR_ZONES + zone_num = 'nodezone'

It's non-trivial to break this 'nodezone' back into node and zone 
numbers.  This patch modifies the way we compute the index to be:
	(node_num << ZONE_SHIFT) | zone_num

This makes it trivial to recover either the node or zone number with a 
simple bitshift.  There are many places in the kernel where we do things 
like: page_zone(page)->zone_pgdat->node_id to determine the node a page 
belongs to.  With this patch we save several pointer dereferences, and 
it all boils down to shifting some bits.

Cheers!

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4133 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.0-vanilla/include/linux/mm.h linux-2.6.0-patched/include/linux/mm.h
--- linux-2.6.0-vanilla/include/linux/mm.h	Wed Dec 17 18:58:05 2003
+++ linux-2.6.0-patched/include/linux/mm.h	Thu Dec 18 14:27:26 2003
@@ -323,20 +323,31 @@ static inline void put_page(struct page 
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - 8)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT + ZONES_SHIFT);
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.0-vanilla/include/linux/mmzone.h linux-2.6.0-patched/include/linux/mmzone.h
--- linux-2.6.0-vanilla/include/linux/mmzone.h	Wed Dec 17 18:58:57 2003
+++ linux-2.6.0-patched/include/linux/mmzone.h	Thu Dec 18 14:27:26 2003
@@ -159,8 +159,10 @@ struct zone {
 #define ZONE_DMA		0
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
-#define MAX_NR_ZONES		3
-#define GFP_ZONEMASK	0x03
+
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* = ceil(log2(MAX_NR_ZONES)) */
+#define GFP_ZONEMASK		0x03
 
 /*
  * One allocation request operates on a zonelist. A zonelist
@@ -310,7 +312,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -327,6 +329,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.0-vanilla/mm/page_alloc.c linux-2.6.0-patched/mm/page_alloc.c
--- linux-2.6.0-vanilla/mm/page_alloc.c	Wed Dec 17 18:58:08 2003
+++ linux-2.6.0-patched/mm/page_alloc.c	Thu Dec 18 14:27:26 2003
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1210,7 +1210,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1251,7 +1251,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];

Re: [PATCH] Simplify node/zone field in page->flags

[Andrew Morton]

Matthew Dobson <colpatch@us.ibm.com> wrote:
>
> Currently we keep track of a pages node & zone in the top 8 bits (on 
> 32-bit arches, 10 bits on 64-bit arches) of page->flags.  We typically 
> compute the field as follows:
> 	node_num * MAX_NR_ZONES + zone_num = 'nodezone'
> 
> It's non-trivial to break this 'nodezone' back into node and zone 
> numbers.  This patch modifies the way we compute the index to be:
> 	(node_num << ZONE_SHIFT) | zone_num
> 
> This makes it trivial to recover either the node or zone number with a 
> simple bitshift.  There are many places in the kernel where we do things 
> like: page_zone(page)->zone_pgdat->node_id to determine the node a page 
> belongs to.  With this patch we save several pointer dereferences, and 
> it all boils down to shifting some bits.

This conflicts with (is a superset of) 

	ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.0-test9/2.6.0-test9-mm5/broken-out/ZONE_SHIFT-from-NODES_SHIFT.patch

I suspect you've sent a replacement patch, yes?  If Jesse is OK with the
new patch I'll do the swap, thanks.

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

Andrew Morton wrote:
> Matthew Dobson <colpatch@us.ibm.com> wrote:
> 
>>Currently we keep track of a pages node & zone in the top 8 bits (on 
>>32-bit arches, 10 bits on 64-bit arches) of page->flags.  We typically 
>>compute the field as follows:
>>	node_num * MAX_NR_ZONES + zone_num = 'nodezone'
>>
>>It's non-trivial to break this 'nodezone' back into node and zone 
>>numbers.  This patch modifies the way we compute the index to be:
>>	(node_num << ZONE_SHIFT) | zone_num
>>
>>This makes it trivial to recover either the node or zone number with a 
>>simple bitshift.  There are many places in the kernel where we do things 
>>like: page_zone(page)->zone_pgdat->node_id to determine the node a page 
>>belongs to.  With this patch we save several pointer dereferences, and 
>>it all boils down to shifting some bits.
> 
> 
> This conflicts with (is a superset of) 
> 
> 	ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.0-test9/2.6.0-test9-mm5/broken-out/ZONE_SHIFT-from-NODES_SHIFT.patch
> 
> I suspect you've sent a replacement patch, yes?  If Jesse is OK with the
> new patch I'll do the swap, thanks.

Jesse had acked the patch in an earlier itteration.  The only thing 
that's changed is some line offsets whilst porting the patch forward.

Jesse (or anyone else?), any objections to this patch as a superset of 
yours?

Cheers!

-Matt

Re: [PATCH] Simplify node/zone field in page->flags

[Jesse Barnes]

On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> Jesse had acked the patch in an earlier itteration.  The only thing 
> that's changed is some line offsets whilst porting the patch forward.
> 
> Jesse (or anyone else?), any objections to this patch as a superset of 
> yours?

No objections here.  Of course, you'll have to rediff against the
current tree since that stuff has been merged for awhile now.  On a
somewhat related note, Martin mentioned that he'd like to get rid of
memblks.  I'm all for that too; they just seem to get in the way.

Jesse

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

Jesse Barnes wrote:
> On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> 
>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>that's changed is some line offsets whilst porting the patch forward.
>>
>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>yours?
> 
> 
> No objections here.  Of course, you'll have to rediff against the
> current tree since that stuff has been merged for awhile now.  On a
> somewhat related note, Martin mentioned that he'd like to get rid of
> memblks.  I'm all for that too; they just seem to get in the way.
> 
> Jesse

Here's an updated version against 2.6.1-rc1.  Small comment fix (there 
are actually up to (MAX_NUMNODES * MAX_NR_ZONES) possible zones total, 
not log2(MAX_NUMNODES * MAX_NR_ZONES) as your comment stated.  That is 
the number of bits necessary to index every possible zone.

After this goes in, we (I) can convert a number of places that are doing 
several pointer dereferences/arithmetic and other things to determine 
which node/zone a page belongs to simply calling 
page_nodenum()/page_zonenum().

Cheers!

-Matt

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

Jesse Barnes wrote:
> On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> 
>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>that's changed is some line offsets whilst porting the patch forward.
>>
>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>yours?
> 
> 
> No objections here.  Of course, you'll have to rediff against the
> current tree since that stuff has been merged for awhile now.  On a
> somewhat related note, Martin mentioned that he'd like to get rid of
> memblks.  I'm all for that too; they just seem to get in the way.
> 
> Jesse
> 

Yeah... didn't actually attatch the patch to that last email, did I? 
Brain slowly transitioning back into "on" mode after a couple weeks 
solidly in the "off" position.

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4416 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mm.h linux-2.6.1-rc1+nodezone/include/linux/mm.h
--- linux-2.6.1-rc1/include/linux/mm.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mm.h	Mon Jan  5 14:26:28 2004
@@ -322,23 +322,33 @@ static inline void put_page(struct page 
 /*
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
- * We'll have up to log2(MAX_NUMNODES * MAX_NR_ZONES) zones
- * total, so we use NODES_SHIFT here to get enough bits.
+ * We'll have up to (MAX_NUMNODES * MAX_NR_ZONES) zones total, 
+ * so we use (MAX_NODES_SHIFT + MAX_ZONES_SHIFT) here to get enough bits.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - NODES_SHIFT - MAX_NR_ZONES_SHIFT)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT + ZONES_SHIFT);
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mmzone.h linux-2.6.1-rc1+nodezone/include/linux/mmzone.h
--- linux-2.6.1-rc1/include/linux/mmzone.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mmzone.h	Mon Jan  5 14:04:46 2004
@@ -160,8 +160,8 @@ struct zone {
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
 
-#define MAX_NR_ZONES		3	/* Sync this with MAX_NR_ZONES_SHIFT */
-#define MAX_NR_ZONES_SHIFT	2	/* ceil(log2(MAX_NR_ZONES)) */
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* ceil(log2(MAX_NR_ZONES)) */
 
 #define GFP_ZONEMASK	0x03
 
@@ -311,7 +311,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -328,6 +328,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/mm/page_alloc.c linux-2.6.1-rc1+nodezone/mm/page_alloc.c
--- linux-2.6.1-rc1/mm/page_alloc.c	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/mm/page_alloc.c	Mon Jan  5 14:04:46 2004
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1212,7 +1212,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1253,7 +1253,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];

Re: [PATCH] Simplify node/zone field in page->flags

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On Tue, 2004-01-06 at 00:33, Matthew Dobson wrote:
> Jesse Barnes wrote:
> > On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> > 
> >>Jesse had acked the patch in an earlier itteration.  The only thing 
> >>that's changed is some line offsets whilst porting the patch forward.
> >>
> >>Jesse (or anyone else?), any objections to this patch as a superset of 
> >>yours?
> > 
> > 
> > No objections here.  Of course, you'll have to rediff against the
> > current tree since that stuff has been merged for awhile now.  On a
> > somewhat related note, Martin mentioned that he'd like to get rid of
> > memblks.  I'm all for that too; they just seem to get in the way.
> > 
> > Jesse
> > 
> 
> Yeah... didn't actually attatch the patch to that last email, did I? 
> Brain slowly transitioning back into "on" mode after a couple weeks 
> solidly in the "off" position.
> 

Get this with gcc-3.3.2 cvs:

--
include/linux/mm.h: In function `page_nodenum':
include/linux/mm.h:337: warning: right shift count >= width of type
include/linux/mm.h:337: warning: suggest parentheses around + or -
inside shift
--

Think we could get those () in to make it more clear and the compiler
happy?


Thanks,

-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

Martin Schlemmer wrote:
> On Tue, 2004-01-06 at 00:33, Matthew Dobson wrote:
> 
>>Jesse Barnes wrote:
>>
>>>On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
>>>
>>>
>>>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>>>that's changed is some line offsets whilst porting the patch forward.
>>>>
>>>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>>>yours?
>>>
>>>
>>>No objections here.  Of course, you'll have to rediff against the
>>>current tree since that stuff has been merged for awhile now.  On a
>>>somewhat related note, Martin mentioned that he'd like to get rid of
>>>memblks.  I'm all for that too; they just seem to get in the way.
>>>
>>>Jesse
>>>
>>
>>Yeah... didn't actually attatch the patch to that last email, did I? 
>>Brain slowly transitioning back into "on" mode after a couple weeks 
>>solidly in the "off" position.
>>
> 
> 
> Get this with gcc-3.3.2 cvs:
> 
> --
> include/linux/mm.h: In function `page_nodenum':
> include/linux/mm.h:337: warning: right shift count >= width of type
> include/linux/mm.h:337: warning: suggest parentheses around + or -
> inside shift
> --
> 
> Think we could get those () in to make it more clear and the compiler
> happy?
> 
> 
> Thanks,

Ok... Not sure how gcc thinks it could be shifting >= width of type? 
page->flags is an unsigned long, NODEZONE_SHIFT + ZONES_SHIFT is by 
definition less than BITS_PER_LONG, but who knows, maybe the parens will 
kill both warnings, eh?

Rediffed to include the parens, and here's a changelog, too!

This patch does the following:
1) Rename ZONE_SHIFT to NODEZONE_SHIFT.  This value is the number
    of bits to shift page->flags to get the node/zone part of the
    bitfield.
2) Add a macro called NODEZONE which takes a node number and zone number
    and returns a 'nodezone', a bitshifted composition of the two.
3) Create page_zonenum & page_nodenum, inline functions to return the
    node/zone a page belongs to with some simple bit twiddling, no
    pointer dereferences necessary.
4) Modify page_zone() and set_page_zone() to use the new NODEZONE_SHIFT.
5) Modify memmap_init_zone() & free_area_init_core() to use the new
    NODEZONE macros.
6) Fix up some comments to reflect the above changes.

Cheers!

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4418 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mm.h linux-2.6.1-rc1+nodezone/include/linux/mm.h
--- linux-2.6.1-rc1/include/linux/mm.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mm.h	Mon Jan  5 16:14:22 2004
@@ -322,23 +322,33 @@ static inline void put_page(struct page 
 /*
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
- * We'll have up to log2(MAX_NUMNODES * MAX_NR_ZONES) zones
- * total, so we use NODES_SHIFT here to get enough bits.
+ * We'll have up to (MAX_NUMNODES * MAX_NR_ZONES) zones total, 
+ * so we use (MAX_NODES_SHIFT + MAX_ZONES_SHIFT) here to get enough bits.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - NODES_SHIFT - MAX_NR_ZONES_SHIFT)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> (NODEZONE_SHIFT + ZONES_SHIFT));
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mmzone.h linux-2.6.1-rc1+nodezone/include/linux/mmzone.h
--- linux-2.6.1-rc1/include/linux/mmzone.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mmzone.h	Mon Jan  5 14:04:46 2004
@@ -160,8 +160,8 @@ struct zone {
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
 
-#define MAX_NR_ZONES		3	/* Sync this with MAX_NR_ZONES_SHIFT */
-#define MAX_NR_ZONES_SHIFT	2	/* ceil(log2(MAX_NR_ZONES)) */
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* ceil(log2(MAX_NR_ZONES)) */
 
 #define GFP_ZONEMASK	0x03
 
@@ -311,7 +311,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -328,6 +328,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/mm/page_alloc.c linux-2.6.1-rc1+nodezone/mm/page_alloc.c
--- linux-2.6.1-rc1/mm/page_alloc.c	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/mm/page_alloc.c	Mon Jan  5 14:04:46 2004
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1212,7 +1212,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1253,7 +1253,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

Martin Schlemmer wrote:
> On Tue, 2004-01-06 at 00:33, Matthew Dobson wrote:
> 
>>Jesse Barnes wrote:
>>
>>>On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
>>>
>>>
>>>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>>>that's changed is some line offsets whilst porting the patch forward.
>>>>
>>>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>>>yours?
>>>
>>>
>>>No objections here.  Of course, you'll have to rediff against the
>>>current tree since that stuff has been merged for awhile now.  On a
>>>somewhat related note, Martin mentioned that he'd like to get rid of
>>>memblks.  I'm all for that too; they just seem to get in the way.
>>>
>>>Jesse
>>>
>>
>>Yeah... didn't actually attatch the patch to that last email, did I? 
>>Brain slowly transitioning back into "on" mode after a couple weeks 
>>solidly in the "off" position.
>>
> 
> 
> Get this with gcc-3.3.2 cvs:
> 
> --
> include/linux/mm.h: In function `page_nodenum':
> include/linux/mm.h:337: warning: right shift count >= width of type
> include/linux/mm.h:337: warning: suggest parentheses around + or -
> inside shift
> --
> 
> Think we could get those () in to make it more clear and the compiler
> happy?
> 
> 
> Thanks,
> 

Figured out what the right shift count message was about.  On UP with 
MAX_NODES_SHIFT = 0, the node bitfield has no length so the bitshift was 
too far.  Setting MAX_NODES_SHIFT = 1 gives the node bitfield a size of 
1, and we just store a 0 there for node 0.

Updated patch attatched.

Thanks!

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4697 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mm.h linux-2.6.1-rc1+nodezone/include/linux/mm.h
--- linux-2.6.1-rc1/include/linux/mm.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mm.h	Mon Jan  5 16:14:22 2004
@@ -322,23 +322,33 @@ static inline void put_page(struct page 
 /*
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
- * We'll have up to log2(MAX_NUMNODES * MAX_NR_ZONES) zones
- * total, so we use NODES_SHIFT here to get enough bits.
+ * We'll have up to (MAX_NUMNODES * MAX_NR_ZONES) zones total, 
+ * so we use (MAX_NODES_SHIFT + MAX_ZONES_SHIFT) here to get enough bits.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - NODES_SHIFT - MAX_NR_ZONES_SHIFT)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> (NODEZONE_SHIFT + ZONES_SHIFT));
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mmzone.h linux-2.6.1-rc1+nodezone/include/linux/mmzone.h
--- linux-2.6.1-rc1/include/linux/mmzone.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mmzone.h	Tue Jan  6 14:02:46 2004
@@ -160,8 +160,8 @@ struct zone {
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
 
-#define MAX_NR_ZONES		3	/* Sync this with MAX_NR_ZONES_SHIFT */
-#define MAX_NR_ZONES_SHIFT	2	/* ceil(log2(MAX_NR_ZONES)) */
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* ceil(log2(MAX_NR_ZONES)) */
 
 #define GFP_ZONEMASK	0x03
 
@@ -303,7 +303,7 @@ int min_free_kbytes_sysctl_handler(struc
 extern struct pglist_data contig_page_data;
 #define NODE_DATA(nid)		(&contig_page_data)
 #define NODE_MEM_MAP(nid)	mem_map
-#define MAX_NODES_SHIFT		0
+#define MAX_NODES_SHIFT		1
 
 #else /* CONFIG_DISCONTIGMEM */
 
@@ -311,7 +311,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -328,6 +328,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/mm/page_alloc.c linux-2.6.1-rc1+nodezone/mm/page_alloc.c
--- linux-2.6.1-rc1/mm/page_alloc.c	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/mm/page_alloc.c	Mon Jan  5 14:04:46 2004
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1212,7 +1212,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1253,7 +1253,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];

Re: [PATCH] Simplify node/zone field in page->flags

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Wed, 2004-01-07 at 00:25, Matthew Dobson wrote:

> > 
> > Get this with gcc-3.3.2 cvs:
> > 
> > --
> > include/linux/mm.h: In function `page_nodenum':
> > include/linux/mm.h:337: warning: right shift count >= width of type
> > include/linux/mm.h:337: warning: suggest parentheses around + or -
> > inside shift
> > --
> > 
> > Think we could get those () in to make it more clear and the compiler
> > happy?
> > 
> > 
> > Thanks,
> > 
> 
> Figured out what the right shift count message was about.  On UP with 
> MAX_NODES_SHIFT = 0, the node bitfield has no length so the bitshift was 
> too far.  Setting MAX_NODES_SHIFT = 1 gives the node bitfield a size of 
> 1, and we just store a 0 there for node 0.
> 
> Updated patch attatched.

Hmm, have smp here:

--
 # grep SMP /usr/src/linux-2.6.1-rc1-bk6/.config
CONFIG_BROKEN_ON_SMP=y
# CONFIG_X86_BIGSMP is not set
CONFIG_SMP=y
CONFIG_X86_FIND_SMP_CONFIG=y
CONFIG_X86_SMP=y
--

but yes, last patch fixes it!


Thanks,

-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Simplify node/zone field in page->flags

[Jesse Barnes]

On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> Jesse had acked the patch in an earlier itteration.  The only thing 
> that's changed is some line offsets whilst porting the patch forward.
> 
> Jesse (or anyone else?), any objections to this patch as a superset of 
> yours?

No objections here.  Of course, you'll have to rediff against the
current tree since that stuff has been merged for awhile now.  On a
somewhat related note, Martin mentioned that he'd like to get rid of
memblks.  I'm all for that too; they just seem to get in the way.

Jesse

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

Jesse Barnes wrote:
> On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> 
>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>that's changed is some line offsets whilst porting the patch forward.
>>
>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>yours?
> 
> 
> No objections here.  Of course, you'll have to rediff against the
> current tree since that stuff has been merged for awhile now.  On a
> somewhat related note, Martin mentioned that he'd like to get rid of
> memblks.  I'm all for that too; they just seem to get in the way.
> 
> Jesse
> 

Yeah... didn't actually attatch the patch to that last email, did I? 
Brain slowly transitioning back into "on" mode after a couple weeks 
solidly in the "off" position.

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4416 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mm.h linux-2.6.1-rc1+nodezone/include/linux/mm.h
--- linux-2.6.1-rc1/include/linux/mm.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mm.h	Mon Jan  5 14:26:28 2004
@@ -322,23 +322,33 @@ static inline void put_page(struct page 
 /*
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
- * We'll have up to log2(MAX_NUMNODES * MAX_NR_ZONES) zones
- * total, so we use NODES_SHIFT here to get enough bits.
+ * We'll have up to (MAX_NUMNODES * MAX_NR_ZONES) zones total, 
+ * so we use (MAX_NODES_SHIFT + MAX_ZONES_SHIFT) here to get enough bits.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - NODES_SHIFT - MAX_NR_ZONES_SHIFT)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT + ZONES_SHIFT);
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mmzone.h linux-2.6.1-rc1+nodezone/include/linux/mmzone.h
--- linux-2.6.1-rc1/include/linux/mmzone.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mmzone.h	Mon Jan  5 14:04:46 2004
@@ -160,8 +160,8 @@ struct zone {
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
 
-#define MAX_NR_ZONES		3	/* Sync this with MAX_NR_ZONES_SHIFT */
-#define MAX_NR_ZONES_SHIFT	2	/* ceil(log2(MAX_NR_ZONES)) */
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* ceil(log2(MAX_NR_ZONES)) */
 
 #define GFP_ZONEMASK	0x03
 
@@ -311,7 +311,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -328,6 +328,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/mm/page_alloc.c linux-2.6.1-rc1+nodezone/mm/page_alloc.c
--- linux-2.6.1-rc1/mm/page_alloc.c	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/mm/page_alloc.c	Mon Jan  5 14:04:46 2004
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1212,7 +1212,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1253,7 +1253,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

Jesse Barnes wrote:
> On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> 
>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>that's changed is some line offsets whilst porting the patch forward.
>>
>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>yours?
> 
> 
> No objections here.  Of course, you'll have to rediff against the
> current tree since that stuff has been merged for awhile now.  On a
> somewhat related note, Martin mentioned that he'd like to get rid of
> memblks.  I'm all for that too; they just seem to get in the way.
> 
> Jesse

Here's an updated version against 2.6.1-rc1.  Small comment fix (there 
are actually up to (MAX_NUMNODES * MAX_NR_ZONES) possible zones total, 
not log2(MAX_NUMNODES * MAX_NR_ZONES) as your comment stated.  That is 
the number of bits necessary to index every possible zone.

After this goes in, we (I) can convert a number of places that are doing 
several pointer dereferences/arithmetic and other things to determine 
which node/zone a page belongs to simply calling 
page_nodenum()/page_zonenum().

Cheers!

-Matt

Re: [PATCH] Simplify node/zone field in page->flags

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On Tue, 2004-01-06 at 00:33, Matthew Dobson wrote:
> Jesse Barnes wrote:
> > On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
> > 
> >>Jesse had acked the patch in an earlier itteration.  The only thing 
> >>that's changed is some line offsets whilst porting the patch forward.
> >>
> >>Jesse (or anyone else?), any objections to this patch as a superset of 
> >>yours?
> > 
> > 
> > No objections here.  Of course, you'll have to rediff against the
> > current tree since that stuff has been merged for awhile now.  On a
> > somewhat related note, Martin mentioned that he'd like to get rid of
> > memblks.  I'm all for that too; they just seem to get in the way.
> > 
> > Jesse
> > 
> 
> Yeah... didn't actually attatch the patch to that last email, did I? 
> Brain slowly transitioning back into "on" mode after a couple weeks 
> solidly in the "off" position.
> 

Get this with gcc-3.3.2 cvs:

--
include/linux/mm.h: In function `page_nodenum':
include/linux/mm.h:337: warning: right shift count >= width of type
include/linux/mm.h:337: warning: suggest parentheses around + or -
inside shift
--

Think we could get those () in to make it more clear and the compiler
happy?


Thanks,

-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Simplify node/zone field in page->flags

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

Martin Schlemmer wrote:
> On Tue, 2004-01-06 at 00:33, Matthew Dobson wrote:
> 
>>Jesse Barnes wrote:
>>
>>>On Mon, Jan 05, 2004 at 01:22:57PM -0800, Matthew Dobson wrote:
>>>
>>>
>>>>Jesse had acked the patch in an earlier itteration.  The only thing 
>>>>that's changed is some line offsets whilst porting the patch forward.
>>>>
>>>>Jesse (or anyone else?), any objections to this patch as a superset of 
>>>>yours?
>>>
>>>
>>>No objections here.  Of course, you'll have to rediff against the
>>>current tree since that stuff has been merged for awhile now.  On a
>>>somewhat related note, Martin mentioned that he'd like to get rid of
>>>memblks.  I'm all for that too; they just seem to get in the way.
>>>
>>>Jesse
>>>
>>
>>Yeah... didn't actually attatch the patch to that last email, did I? 
>>Brain slowly transitioning back into "on" mode after a couple weeks 
>>solidly in the "off" position.
>>
> 
> 
> Get this with gcc-3.3.2 cvs:
> 
> --
> include/linux/mm.h: In function `page_nodenum':
> include/linux/mm.h:337: warning: right shift count >= width of type
> include/linux/mm.h:337: warning: suggest parentheses around + or -
> inside shift
> --
> 
> Think we could get those () in to make it more clear and the compiler
> happy?
> 
> 
> Thanks,

Ok... Not sure how gcc thinks it could be shifting >= width of type? 
page->flags is an unsigned long, NODEZONE_SHIFT + ZONES_SHIFT is by 
definition less than BITS_PER_LONG, but who knows, maybe the parens will 
kill both warnings, eh?

Rediffed to include the parens, and here's a changelog, too!

This patch does the following:
1) Rename ZONE_SHIFT to NODEZONE_SHIFT.  This value is the number
    of bits to shift page->flags to get the node/zone part of the
    bitfield.
2) Add a macro called NODEZONE which takes a node number and zone number
    and returns a 'nodezone', a bitshifted composition of the two.
3) Create page_zonenum & page_nodenum, inline functions to return the
    node/zone a page belongs to with some simple bit twiddling, no
    pointer dereferences necessary.
4) Modify page_zone() and set_page_zone() to use the new NODEZONE_SHIFT.
5) Modify memmap_init_zone() & free_area_init_core() to use the new
    NODEZONE macros.
6) Fix up some comments to reflect the above changes.

Cheers!

-Matt

[-- Attachment #2: nodezone.patch --]
[-- Type: text/plain, Size: 4418 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mm.h linux-2.6.1-rc1+nodezone/include/linux/mm.h
--- linux-2.6.1-rc1/include/linux/mm.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mm.h	Mon Jan  5 16:14:22 2004
@@ -322,23 +322,33 @@ static inline void put_page(struct page 
 /*
  * The zone field is never updated after free_area_init_core()
  * sets it, so none of the operations on it need to be atomic.
- * We'll have up to log2(MAX_NUMNODES * MAX_NR_ZONES) zones
- * total, so we use NODES_SHIFT here to get enough bits.
+ * We'll have up to (MAX_NUMNODES * MAX_NR_ZONES) zones total, 
+ * so we use (MAX_NODES_SHIFT + MAX_ZONES_SHIFT) here to get enough bits.
  */
-#define ZONE_SHIFT (BITS_PER_LONG - NODES_SHIFT - MAX_NR_ZONES_SHIFT)
+#define NODEZONE_SHIFT (BITS_PER_LONG - MAX_NODES_SHIFT - MAX_ZONES_SHIFT)
+#define NODEZONE(node, zone)	((node << ZONES_SHIFT) | zone)
+
+static inline unsigned long page_zonenum(struct page *page)
+{
+	return (page->flags >> NODEZONE_SHIFT) & (~(~0UL << ZONES_SHIFT));
+}
+static inline unsigned long page_nodenum(struct page *page)
+{
+	return (page->flags >> (NODEZONE_SHIFT + ZONES_SHIFT));
+}
 
 struct zone;
 extern struct zone *zone_table[];
 
 static inline struct zone *page_zone(struct page *page)
 {
-	return zone_table[page->flags >> ZONE_SHIFT];
+	return zone_table[page->flags >> NODEZONE_SHIFT];
 }
 
-static inline void set_page_zone(struct page *page, unsigned long zone_num)
+static inline void set_page_zone(struct page *page, unsigned long nodezone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
-	page->flags |= zone_num << ZONE_SHIFT;
+	page->flags &= ~(~0UL << NODEZONE_SHIFT);
+	page->flags |= nodezone_num << NODEZONE_SHIFT;
 }
 
 #ifndef CONFIG_DISCONTIGMEM
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/include/linux/mmzone.h linux-2.6.1-rc1+nodezone/include/linux/mmzone.h
--- linux-2.6.1-rc1/include/linux/mmzone.h	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/include/linux/mmzone.h	Mon Jan  5 14:04:46 2004
@@ -160,8 +160,8 @@ struct zone {
 #define ZONE_NORMAL		1
 #define ZONE_HIGHMEM		2
 
-#define MAX_NR_ZONES		3	/* Sync this with MAX_NR_ZONES_SHIFT */
-#define MAX_NR_ZONES_SHIFT	2	/* ceil(log2(MAX_NR_ZONES)) */
+#define MAX_NR_ZONES		3	/* Sync this with ZONES_SHIFT */
+#define ZONES_SHIFT		2	/* ceil(log2(MAX_NR_ZONES)) */
 
 #define GFP_ZONEMASK	0x03
 
@@ -311,7 +311,7 @@ extern struct pglist_data contig_page_da
 
 #if BITS_PER_LONG == 32
 /*
- * with 32 bit flags field, page->zone is currently 8 bits.
+ * with 32 bit page->flags field, we reserve 8 bits for node/zone info.
  * there are 3 zones (2 bits) and this leaves 8-2=6 bits for nodes.
  */
 #define MAX_NODES_SHIFT		6
@@ -328,6 +328,13 @@ extern struct pglist_data contig_page_da
 #error NODES_SHIFT > MAX_NODES_SHIFT
 #endif
 
+/* There are currently 3 zones: DMA, Normal & Highmem, thus we need 2 bits */
+#define MAX_ZONES_SHIFT		2
+
+#if ZONES_SHIFT > MAX_ZONES_SHIFT
+#error ZONES_SHIFT > MAX_ZONES_SHIFT
+#endif
+
 extern DECLARE_BITMAP(node_online_map, MAX_NUMNODES);
 extern DECLARE_BITMAP(memblk_online_map, MAX_NR_MEMBLKS);
 
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.1-rc1/mm/page_alloc.c linux-2.6.1-rc1+nodezone/mm/page_alloc.c
--- linux-2.6.1-rc1/mm/page_alloc.c	Mon Jan  5 12:36:24 2004
+++ linux-2.6.1-rc1+nodezone/mm/page_alloc.c	Mon Jan  5 14:04:46 2004
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(nr_swap_pages);
  * Used by page_zone() to look up the address of the struct zone whose
  * id is encoded in the upper bits of page->flags
  */
-struct zone *zone_table[MAX_NR_ZONES*MAX_NUMNODES];
+struct zone *zone_table[1 << (ZONES_SHIFT + NODES_SHIFT)];
 EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
@@ -1212,7 +1212,7 @@ void __init memmap_init_zone(struct page
 	struct page *page;
 
 	for (page = start; page < (start + size); page++) {
-		set_page_zone(page, nid * MAX_NR_ZONES + zone);
+		set_page_zone(page, NODEZONE(nid, zone));
 		set_page_count(page, 0);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->list);
@@ -1253,7 +1253,7 @@ static void __init free_area_init_core(s
 		unsigned long size, realsize;
 		unsigned long batch;
 
-		zone_table[nid * MAX_NR_ZONES + j] = zone;
+		zone_table[NODEZONE(nid, j)] = zone;
 		realsize = size = zones_size[j];
 		if (zholes_size)
 			realsize -= zholes_size[j];