[PATCH] sparsemem fix

[PATCH] sparsemem fix

[Atsushi Nemoto]

1. MIPS should select SPARSEMEM_STATIC since allocating bootmem in
   memory_present() will corrupt bootmap area.
2. pfn_valid() for SPARSEMEM is defined in linux/mmzone.h

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index f151a7e..879a19c 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1690,6 +1690,7 @@ config ARCH_DISCONTIGMEM_ENABLE
 
 config ARCH_SPARSEMEM_ENABLE
 	bool
+	select SPARSEMEM_STATIC
 
 config NUMA
 	bool "NUMA Support"
diff --git a/include/asm-mips/page.h b/include/asm-mips/page.h
index 6b97744..6ed1151 100644
--- a/include/asm-mips/page.h
+++ b/include/asm-mips/page.h
@@ -138,16 +138,14 @@ #define __va(x)			((void *)((unsigned lo
 
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
-#ifndef CONFIG_SPARSEMEM
-#ifndef CONFIG_NEED_MULTIPLE_NODES
-#define pfn_valid(pfn)		((pfn) < max_mapnr)
-#endif
-#endif
-
 #ifdef CONFIG_FLATMEM
 
 #define pfn_valid(pfn)		((pfn) < max_mapnr)
 
+#elif defined(CONFIG_SPARSEMEM)
+
+/* pfn_valid is defined in linux/mmzone.h */
+
 #elif defined(CONFIG_NEED_MULTIPLE_NODES)
 
 #define pfn_valid(pfn)							\
@@ -159,8 +157,6 @@ ({									\
 	            : 0);						\
 })
 
-#else
-#error Provide a definition of pfn_valid
 #endif
 
 #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)

Re: [PATCH] sparsemem fix

[Chad Reese]

I believe Ralf committed a cleaned up version of the patch I created 
5/23/2006. It called memory_present() after the first bootmap memory was 
created. I've been using this and dynamic sparsemem on Mips64 for a 
while now.

Hope this helps,

Chad

Atsushi Nemoto wrote:

>1. MIPS should select SPARSEMEM_STATIC since allocating bootmem in
>   memory_present() will corrupt bootmap area.
>2. pfn_valid() for SPARSEMEM is defined in linux/mmzone.h
>
>Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>
>diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>index f151a7e..879a19c 100644
>--- a/arch/mips/Kconfig
>+++ b/arch/mips/Kconfig
>@@ -1690,6 +1690,7 @@ config ARCH_DISCONTIGMEM_ENABLE
> 
> config ARCH_SPARSEMEM_ENABLE
> 	bool
>+	select SPARSEMEM_STATIC
> 
> config NUMA
> 	bool "NUMA Support"
>diff --git a/include/asm-mips/page.h b/include/asm-mips/page.h
>index 6b97744..6ed1151 100644
>--- a/include/asm-mips/page.h
>+++ b/include/asm-mips/page.h
>@@ -138,16 +138,14 @@ #define __va(x)			((void *)((unsigned lo
> 
> #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
> 
>-#ifndef CONFIG_SPARSEMEM
>-#ifndef CONFIG_NEED_MULTIPLE_NODES
>-#define pfn_valid(pfn)		((pfn) < max_mapnr)
>-#endif
>-#endif
>-
> #ifdef CONFIG_FLATMEM
> 
> #define pfn_valid(pfn)		((pfn) < max_mapnr)
> 
>+#elif defined(CONFIG_SPARSEMEM)
>+
>+/* pfn_valid is defined in linux/mmzone.h */
>+
> #elif defined(CONFIG_NEED_MULTIPLE_NODES)
> 
> #define pfn_valid(pfn)							\
>@@ -159,8 +157,6 @@ ({									\
> 	            : 0);						\
> })
> 
>-#else
>-#error Provide a definition of pfn_valid
> #endif
> 
> #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>
>  
>

Re: [PATCH] sparsemem fix

[Atsushi Nemoto]

On Tue, 04 Jul 2006 10:03:35 -0700, Chad Reese <kreese@caviumnetworks.com> wrote:
> I believe Ralf committed a cleaned up version of the patch I created 
> 5/23/2006. It called memory_present() after the first bootmap memory was 
> created. I've been using this and dynamic sparsemem on Mips64 for a 
> while now.

It is not enough.  If you want to use SPARSEMEM_EXTREME, do not call
memory_present() _before_ reserve_bootmem().

For SPARSEMEM_EXTREME, memory_present() try to allocate bootmem, but
first area of bootmem must be reserved for bootmap before any
allocation.

The alloc_bootmem_node try to allocate upper (>16MB) page first, then
try lower page.  So if the first memory area was smaller then 16MB
SPARSEMEM_EXTREME would not work.

Also, SPARSEMEM_STATIC will be a bit faster then SPARSEMEM_EXTREME.
The mm/Kconfig warns about mem_section[] size, but static
mem_section[] size is just 1KB for MIPS.  No problem.  :-)

---
Atsushi Nemoto

Re: [PATCH] sparsemem fix

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> 1. MIPS should select SPARSEMEM_STATIC since allocating bootmem in
>    memory_present() will corrupt bootmap area.
> 2. pfn_valid() for SPARSEMEM is defined in linux/mmzone.h
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index f151a7e..879a19c 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -1690,6 +1690,7 @@ config ARCH_DISCONTIGMEM_ENABLE
>  
>  config ARCH_SPARSEMEM_ENABLE
>  	bool
> +	select SPARSEMEM_STATIC
>  
>  config NUMA
>  	bool "NUMA Support"
> diff --git a/include/asm-mips/page.h b/include/asm-mips/page.h
> index 6b97744..6ed1151 100644
> --- a/include/asm-mips/page.h
> +++ b/include/asm-mips/page.h
> @@ -138,16 +138,14 @@ #define __va(x)			((void *)((unsigned lo
>  
>  #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
>  
> -#ifndef CONFIG_SPARSEMEM
> -#ifndef CONFIG_NEED_MULTIPLE_NODES
> -#define pfn_valid(pfn)		((pfn) < max_mapnr)
> -#endif
> -#endif
> -
>  #ifdef CONFIG_FLATMEM
>  
>  #define pfn_valid(pfn)		((pfn) < max_mapnr)
>  

In flatmem case, I would define pfn_valid like:

#define pfn_valid(pfn)          ((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)

> +#elif defined(CONFIG_SPARSEMEM)
> +
> +/* pfn_valid is defined in linux/mmzone.h */
> +
>  #elif defined(CONFIG_NEED_MULTIPLE_NODES)

why not using:

#elif defined(CONFIG_DISCONTIGMEM) || defined(CONFIG_NUMA)

hence, we would have all memory model cases.

For now it seems to be implemented only in sgi-ip27 machine. Maybe we should
make things clear by adding:

#ifdef CONFIG_SGI_IP27
#define pfn_valid	[...]
#else
#error discontigmem model is only supported by sgi-ip27 platforms.
#error Please try to implement a generic solution if you plan to
#error use this memory model. Good luck ;)
#endif /* CONFIG_SGI_IP27 */

>  
>  #define pfn_valid(pfn)							\
> @@ -159,8 +157,6 @@ ({									\
>  	            : 0);						\
>  })
>  
> -#else
> -#error Provide a definition of pfn_valid
>  #endif

maybe this would be better too ?

#else

#error Unknow memory model, provide a definition of pfn_valid

#endif

>  
>  #define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> 
> 

Re: [PATCH] sparsemem fix

[Atsushi Nemoto]

On Wed, 05 Jul 2006 10:35:28 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> >  #elif defined(CONFIG_NEED_MULTIPLE_NODES)
> 
> why not using:
> 
> #elif defined(CONFIG_DISCONTIGMEM) || defined(CONFIG_NUMA)
> 
> hence, we would have all memory model cases.

While NEED_MULTIPLE_NODES is defined if DISCONTIGMEM || NUMA, it seems
no difference.

> For now it seems to be implemented only in sgi-ip27 machine. Maybe we should
> make things clear by adding:
> 
> #ifdef CONFIG_SGI_IP27
> #define pfn_valid	[...]
> #else
> #error discontigmem model is only supported by sgi-ip27 platforms.
> #error Please try to implement a generic solution if you plan to
> #error use this memory model. Good luck ;)
> #endif /* CONFIG_SGI_IP27 */

Though the pfn_valid() is only used by ip27 for now, I suppose it
could be used other NUMA systems (not sure).

---
Atsushi Nemoto

Re: [PATCH] sparsemem fix

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Wed, 05 Jul 2006 10:35:28 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>>  #elif defined(CONFIG_NEED_MULTIPLE_NODES)
>> why not using:
>>
>> #elif defined(CONFIG_DISCONTIGMEM) || defined(CONFIG_NUMA)
>>
>> hence, we would have all memory model cases.
> 
> While NEED_MULTIPLE_NODES is defined if DISCONTIGMEM || NUMA, it seems
> no difference.
> 

well, in the previous case the reader sees a case for _all_ memory models.
In your case the reader needs to know that NEED_MULTIPLE_NODES is defined
as (DISCONTIGMEM || NUMA).

>> For now it seems to be implemented only in sgi-ip27 machine. Maybe we should
>> make things clear by adding:
>>
>> #ifdef CONFIG_SGI_IP27
>> #define pfn_valid	[...]
>> #else
>> #error discontigmem model is only supported by sgi-ip27 platforms.
>> #error Please try to implement a generic solution if you plan to
>> #error use this memory model. Good luck ;)
>> #endif /* CONFIG_SGI_IP27 */
> 
> Though the pfn_valid() is only used by ip27 for now, I suppose it
> could be used other NUMA systems (not sure).
> 

no the code related to NUMA is embedded in ip27 directory. So if
someone has another NUMA system, she should (a) copy all the stuff
in its platform directory or (b) make a generic solution maybe based
on ip27 one for all others NUMA platforms.  But in the second case,
the NUMA implementation is going to be modified heavily (a guess)
and probably same for pfn_valid definition.

The previous change makes things clear: for now, you can't use
pfn_valid when NUMA or DISCONTIGMEM configs without some reworks.


		Franck

Re: [PATCH] sparsemem fix

[Ralf Baechle]

On Wed, Jul 05, 2006 at 12:51:25PM +0200, Franck Bui-Huu wrote:

> no the code related to NUMA is embedded in ip27 directory. So if
> someone has another NUMA system, she should (a) copy all the stuff
> in its platform directory or (b) make a generic solution maybe based
> on ip27 one for all others NUMA platforms.  But in the second case,
> the NUMA implementation is going to be modified heavily (a guess)
> and probably same for pfn_valid definition.
> 
> The previous change makes things clear: for now, you can't use
> pfn_valid when NUMA or DISCONTIGMEM configs without some reworks.

Most of the related IP27 code bits are for initialization.

  Ralf

Re: [PATCH] sparsemem fix

[Ralf Baechle]

On Wed, Jul 05, 2006 at 07:20:54PM +0900, Atsushi Nemoto wrote:

> > For now it seems to be implemented only in sgi-ip27 machine. Maybe we should
> > make things clear by adding:
> > 
> > #ifdef CONFIG_SGI_IP27
> > #define pfn_valid	[...]
> > #else

The fact that the code is only used on IP27 doesn't mean it is IP27-specific.

> > #error discontigmem model is only supported by sgi-ip27 platforms.
> > #error Please try to implement a generic solution if you plan to
> > #error use this memory model. Good luck ;)
> > #endif /* CONFIG_SGI_IP27 */
> 
> Though the pfn_valid() is only used by ip27 for now, I suppose it
> could be used other NUMA systems (not sure).

Yes, and there will be one or two more NUMA systems.

  Ralf

Re: [PATCH] sparsemem fix

[Franck Bui-Huu]

2006/7/6, Ralf Baechle <ralf@linux-mips.org>:
> On Wed, Jul 05, 2006 at 07:20:54PM +0900, Atsushi Nemoto wrote:
>
> > > For now it seems to be implemented only in sgi-ip27 machine. Maybe we should
> > > make things clear by adding:
> > >
> > > #ifdef CONFIG_SGI_IP27
> > > #define pfn_valid   [...]
> > > #else
>
> The fact that the code is only used on IP27 doesn't mean it is IP27-specific.
>

but the code seems to be in arch/mips/sgi-ip27, no ?

BTW, Ralf, are there any needs for MIPS to support platforms whose
memory start is not 0 ? I have made a patch for that, and wondering if
it's worth to post it on the list...

-- 
               Franck

Re: [PATCH] sparsemem fix

[Ralf Baechle]

On Sat, Jul 08, 2006 at 04:47:39PM +0200, Franck Bui-Huu wrote:

> but the code seems to be in arch/mips/sgi-ip27, no ?
> 
> BTW, Ralf, are there any needs for MIPS to support platforms whose
> memory start is not 0 ? I have made a patch for that, and wondering if
> it's worth to post it on the list...

Yes, while it's relativly rare such platforms do exist and they currently
pay the price of wasting some memory for the allocation of unused entries
in mem_map[].

  Ralf

Re: [PATCH] sparsemem fix

[Franck Bui-Huu]

Ralf Baechle wrote:
> On Sat, Jul 08, 2006 at 04:47:39PM +0200, Franck Bui-Huu wrote:
> 
>> but the code seems to be in arch/mips/sgi-ip27, no ?
>>
>> BTW, Ralf, are there any needs for MIPS to support platforms whose
>> memory start is not 0 ? I have made a patch for that, and wondering if
>> it's worth to post it on the list...
> 
> Yes, while it's relativly rare such platforms do exist and they currently
> pay the price of wasting some memory for the allocation of unused entries
> in mem_map[].
> 

Ok I'll send that this week. I remember to have to replace CPHYSADDR()
macro with __pa() in boot init code still I don't if it's correct.

Can you tell me why we can't always use __pa() ?

Thanks

		Franck