include/asm-arm/memory.h changes break zaurus sl-5500 boot

include/asm-arm/memory.h changes break zaurus sl-5500 boot

[Pavel Machek]

Hi!

This reverts this (and one more) patch, and fixes boot on
collie. Without this patch, I get some fairly strange warnings about
shift bigger than page size in pfn_to_page().

								Pavel

commit 7eb98a2f3b605d8a11434d855b58d828393ea533
tree 4643bb90d8fe3e48ca8d042286ca3d55b8560d45
parent 1c05dda2b6f025267ab79a267e0a84628a3760e1
author KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Mon, 27 Mar
2006 01:15:37 -0800
committer Linus Torvalds <torvalds@g5.osdl.org> Mon, 27 Mar 2006
08:44:44 -0800

    [PATCH] unify pfn_to_page: arm pfn_to_page

    ARM can use generic funcs.
    PFN_TO_NID, LOCAL_MAP_NR are defined by sub-archs.

    Signed-off-by: KAMEZAWA Hirotuki <kamezawa.hiroyu@jp.fujitsu.com>
    Cc: Russell King <rmk@arm.linux.org.uk>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>


diff --git a/include/asm-alpha/mmzone.h b/include/asm-alpha/mmzone.h
index 192d80c..c900439 100644
--- a/include/asm-alpha/mmzone.h
+++ b/include/asm-alpha/mmzone.h
@@ -83,7 +83,8 @@ PLAT_NODE_DATA_LOCALNR(unsigned long p, 
 	pte_t pte;                                                           \
 	unsigned long pfn;                                                   \
 									     \
-	pfn = page_to_pfn(page) << 32; \
+	pfn = ((unsigned long)((page)-page_zone(page)->zone_mem_map)) << 32; \
+	pfn += page_zone(page)->zone_start_pfn << 32;			     \
 	pte_val(pte) = pfn | pgprot_val(pgprot);			     \
 									     \
 	pte;								     \
diff --git a/include/asm-arm/memory.h b/include/asm-arm/memory.h
index afa5c3e..b4e1146 100644
--- a/include/asm-arm/memory.h
+++ b/include/asm-arm/memory.h
@@ -172,7 +172,9 @@ static inline __deprecated void *bus_to_
  *  virt_addr_valid(k)	indicates whether a virtual address is valid
  */
 #ifndef CONFIG_DISCONTIGMEM
-#define ARCH_PFN_OFFSET		(PHYS_PFN_OFFSET)
+
+#define page_to_pfn(page)	(((page) - mem_map) + PHYS_PFN_OFFSET)
+#define pfn_to_page(pfn)	((mem_map + (pfn)) - PHYS_PFN_OFFSET)
 #define pfn_valid(pfn)		((pfn) >= PHYS_PFN_OFFSET && (pfn) < (PHYS_PFN_OFFSET + max_mapnr))
 
 #define virt_to_page(kaddr)	(pfn_to_page(__pa(kaddr) >> PAGE_SHIFT))
@@ -187,8 +189,13 @@ static inline __deprecated void *bus_to_
  * around in memory.
  */
 #include <linux/numa.h>
-#define arch_pfn_to_nid(pfn)	(PFN_TO_NID(pfn))
-#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
+
+#define page_to_pfn(page)					\
+	(( (page) - page_zone(page)->zone_mem_map)		\
+	  + page_zone(page)->zone_start_pfn)
+
+#define pfn_to_page(pfn)					\
+	(PFN_TO_MAPBASE(pfn) + LOCAL_MAP_NR((pfn) << PAGE_SHIFT))
 
 #define pfn_valid(pfn)						\
 	({							\
@@ -236,6 +243,4 @@ static inline __deprecated void *bus_to_
 
 #endif
 
-#include <asm-generic/memory_model.h>
-
 #endif
diff --git a/include/asm-generic/memory_model.h b/include/asm-generic/memory_model.h
index 0cfb086..a7bb497 100644
--- a/include/asm-generic/memory_model.h
+++ b/include/asm-generic/memory_model.h
@@ -45,11 +45,11 @@ extern unsigned long page_to_pfn(struct 
 	NODE_DATA(__nid)->node_mem_map + arch_local_page_offset(__pfn, __nid);\
 })
 
-#define page_to_pfn(pg)							\
-({	struct page *__pg = (pg);					\
-	struct pglist_data *__pgdat = NODE_DATA(page_to_nid(__pg));	\
-	(unsigned long)(__pg - __pgdat->node_mem_map) +			\
-	 __pgdat->node_start_pfn;					\
+#define page_to_pfn(pg)			\
+({	struct page *__pg = (pg);		\
+	struct zone *__zone = page_zone(__pg);	\
+	(unsigned long)(__pg - __zone->zone_mem_map) +	\
+	 __zone->zone_start_pfn;			\
 })
 
 #elif defined(CONFIG_SPARSEMEM)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index b5c2112..9d2b23e 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -226,6 +226,7 @@ struct zone {
 	 * Discontig memory support fields.
 	 */
 	struct pglist_data	*zone_pgdat;
+	struct page		*zone_mem_map;
 	/* zone_start_pfn == zone_start_paddr >> PAGE_SHIFT */
 	unsigned long		zone_start_pfn;
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index dc523a1..519724d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2041,6 +2041,7 @@ static __meminit void init_currently_emp
 	zone_wait_table_init(zone, size);
 	pgdat->nr_zones = zone_idx(zone) + 1;
 
+	zone->zone_mem_map = pfn_to_page(zone_start_pfn);
 	zone->zone_start_pfn = zone_start_pfn;
 
 	memmap_init(size, pgdat->node_id, zone_idx(zone), zone_start_pfn);
@@ -2767,8 +2768,9 @@ struct page *pfn_to_page(unsigned long p
 }
 unsigned long page_to_pfn(struct page *page)
 {
-	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
-	return (page - pgdat->node_mem_map) + pgdat->node_start_pfn;
+	struct zone *zone = page_zone(page);
+	return (page - zone->zone_mem_map) + zone->zone_start_pfn;
+
 }
 #elif defined(CONFIG_SPARSEMEM)
 struct page *pfn_to_page(unsigned long pfn)

-- 
Picture of sleeping (Linux) penguin wanted...

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[Russell King]

On Sun, Apr 02, 2006 at 11:00:03PM +0200, Pavel Machek wrote:
> This reverts this (and one more) patch, and fixes boot on
> collie. Without this patch, I get some fairly strange warnings about
> shift bigger than page size in pfn_to_page().

Not surprising given this gem:

> -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))

PAGE_OFFSET being 3GB - that's one hell of a shift value!

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[Pavel Machek]

On Ne 02-04-06 23:08:07, Russell King wrote:
> On Sun, Apr 02, 2006 at 11:00:03PM +0200, Pavel Machek wrote:
> > This reverts this (and one more) patch, and fixes boot on
> > collie. Without this patch, I get some fairly strange warnings about
> > shift bigger than page size in pfn_to_page().
> 
> Not surprising given this gem:
> 
> > -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> 
> PAGE_OFFSET being 3GB - that's one hell of a shift value!

Unfortunately this is mainline now. Is there some better fix than
simply reverting the offending patches?
							Pavel
-- 
Picture of sleeping (Linux) penguin wanted...

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[KAMEZAWA Hiroyuki]

On Mon, 3 Apr 2006 00:23:14 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Ne 02-04-06 23:08:07, Russell King wrote:
> > On Sun, Apr 02, 2006 at 11:00:03PM +0200, Pavel Machek wrote:
> > > This reverts this (and one more) patch, and fixes boot on
> > > collie. Without this patch, I get some fairly strange warnings about
> > > shift bigger than page size in pfn_to_page().
> > 
> > Not surprising given this gem:
> > 
> > > -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> > 
> > PAGE_OFFSET being 3GB - that's one hell of a shift value!
> 
stupid...PAGE_OFFSET is offset.. not shift.

LOCAL_MAP_NR((pfn) << PAGE_SHIFT)  ?? what does file include this ?
I'll look into.

- Kame

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[KAMEZAWA Hiroyuki]

On Mon, 3 Apr 2006 00:23:14 +0200
Pavel Machek <pavel@ucw.cz> wrote:
> > Not surprising given this gem:
> > 
> > > -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> > 
> > PAGE_OFFSET being 3GB - that's one hell of a shift value!
> 
> Unfortunately this is mainline now. Is there some better fix than
> simply reverting the offending patches?

Maybe this one will fix (against 2.6.16-mm2)

LOCAL_MAP_NR(kaddr) returns page offset in a node.


-Kame
==
Index: linux-2.6.16-mm2/include/asm-arm/memory.h
===================================================================
--- linux-2.6.16-mm2.orig/include/asm-arm/memory.h
+++ linux-2.6.16-mm2/include/asm-arm/memory.h
@@ -188,7 +188,7 @@ static inline __deprecated void *bus_to_
  */
 #include <linux/numa.h>
 #define arch_pfn_to_nid(pfn)	(PFN_TO_NID(pfn))
-#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
+#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR(__va((pfn) << PAGE_SHIFT)))
 
 #define pfn_valid(pfn)						\
 	({							\

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[Russell King]

On Mon, Apr 03, 2006 at 09:15:04AM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 3 Apr 2006 00:23:14 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> > > Not surprising given this gem:
> > > 
> > > > -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> > > 
> > > PAGE_OFFSET being 3GB - that's one hell of a shift value!
> > 
> > Unfortunately this is mainline now. Is there some better fix than
> > simply reverting the offending patches?
> 
> Maybe this one will fix (against 2.6.16-mm2)
> 
> LOCAL_MAP_NR(kaddr) returns page offset in a node.

LOCAL_MAP_NR does not take a kernel virtual address.  If you look at how
it's defined (Eg):

#define LOCAL_MAP_NR(addr) \
        (((unsigned long)(addr) & 0x07ffffff) >> PAGE_SHIFT)

then you will notice that it's actually converting an offset into a node
into an index.  This means that converting a PFN to a virtual address is
just a complete and utter waste of time.

So:

> -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> +#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR(__va((pfn) << PAGE_SHIFT)))

this should be:

#define arch_local_page_offset(pfn, nid) LOCAL_MAP_NR((pfn) << PAGE_SHIFT)

Please also avoid the over-use of parens.  It's a disease which just makes
things harder to read (eg, the have I got enough close parens at the end
of the line to balance.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[KAMEZAWA Hiroyuki]

On Mon, 3 Apr 2006 08:36:53 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Mon, Apr 03, 2006 at 09:15:04AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 3 Apr 2006 00:23:14 +0200
> > Pavel Machek <pavel@ucw.cz> wrote:
> > > > Not surprising given this gem:
> > > > 
> > > > > -#define arch_local_page_offset(pfn, nid) (LOCAL_MAP_NR((pfn) << PAGE_OFFSET))
> > > > 
> > > > PAGE_OFFSET being 3GB - that's one hell of a shift value!
> > > 
> > > Unfortunately this is mainline now. Is there some better fix than
> > > simply reverting the offending patches?
> > 
> > Maybe this one will fix (against 2.6.16-mm2)
> > 
> > LOCAL_MAP_NR(kaddr) returns page offset in a node.
> 
> LOCAL_MAP_NR does not take a kernel virtual address.  If you look at how
> it's defined (Eg):
> 
> #define LOCAL_MAP_NR(addr) \
>         (((unsigned long)(addr) & 0x07ffffff) >> PAGE_SHIFT)
> 

Hmm..from include/asm-arm/arch-clps711x/memory.h

==
/*
 * Given a kaddr, LOCAL_MAR_NR finds the owning node of the memory
 * and returns the index corresponding to the appropriate page in the
 * node's mem_map.
 */
#define LOCAL_MAP_NR(addr) \
        (((unsigned long)(addr) & (NODE_MAX_MEM_SIZE - 1)) >> PAGE_SHIFT)
==

Is this comment wrong ???

I already posted patch against 2.6.17-rc1. so, please NACK for it.
sorry for annoying.

Thanks,
-- Kame

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[KAMEZAWA Hiroyuki]

On Mon, 3 Apr 2006 16:44:34 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 3 Apr 2006 08:36:53 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> Hmm..from include/asm-arm/arch-clps711x/memory.h
> 
> ==
> /*
>  * Given a kaddr, LOCAL_MAR_NR finds the owning node of the memory
>  * and returns the index corresponding to the appropriate page in the
>  * node's mem_map.
>  */
> #define LOCAL_MAP_NR(addr) \
>         (((unsigned long)(addr) & (NODE_MAX_MEM_SIZE - 1)) >> PAGE_SHIFT)
> ==
> 
> Is this comment wrong ???
> 
Russell-san

All of sub-arch's comment says "Given a kaddr" but all of them just uses "offset".
So, paddr can be given as arg. 
__va()(or pfn_to_kaddr()) adds unnecessary calcs, so paddr should be used instead of kaddr.
Right ?

-Kame

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[Russell King]

On Mon, Apr 03, 2006 at 05:56:03PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 3 Apr 2006 16:44:34 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > On Mon, 3 Apr 2006 08:36:53 +0100
> > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > Hmm..from include/asm-arm/arch-clps711x/memory.h
> > 
> > ==
> > /*
> >  * Given a kaddr, LOCAL_MAR_NR finds the owning node of the memory
> >  * and returns the index corresponding to the appropriate page in the
> >  * node's mem_map.
> >  */
> > #define LOCAL_MAP_NR(addr) \
> >         (((unsigned long)(addr) & (NODE_MAX_MEM_SIZE - 1)) >> PAGE_SHIFT)
> > ==
> > 
> > Is this comment wrong ???

The comment isn't entirely accurate.

> All of sub-arch's comment says "Given a kaddr" but all of them just uses
> "offset".  So, paddr can be given as arg. 

Yes.

> __va()(or pfn_to_kaddr()) adds unnecessary calcs, so paddr should be used
> instead of kaddr.

The problem with __va() and pfn_to_kaddr() is that it adds unnecessary
calculation which the compiler can not optimise away.

>From the use of LOCAL_MAP_NR(), we know that it's returning an offset
into a particular node, and that there's a 1:1 relationship between
the virtual and physical addresses within nodes.  That means that
LOCAL_MAP_NR() can take either a virtual address, a physical address,
or even a byte offset into the node.

In turn, that means that we can pass it whatever address we happen to
have available at the time, or whichever is simpler to calculate.

In the cases you're looking at (pfn to map number) giving it pfn <<
PAGE_SHIFT is entirely sufficient, and is in fact optimal for
performance.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: include/asm-arm/memory.h changes break zaurus sl-5500 boot

[KAMEZAWA Hiroyuki]

On Mon, 3 Apr 2006 10:02:07 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> In turn, that means that we can pass it whatever address we happen to
> have available at the time, or whichever is simpler to calculate.
> 
interesting :)

> In the cases you're looking at (pfn to map number) giving it pfn <<
> PAGE_SHIFT is entirely sufficient, and is in fact optimal for
> performance.

Ok, I'll post fixed patch, which uses LOCAL_MAP_NR((pfn) << PAGE_SHIFT)

Thank you for advices!

--Kame