[PATCH] gurantee DMA area for alloc_bootmem_low()

[PATCH] gurantee DMA area for alloc_bootmem_low()

[Yasunori Goto]

Hello.

This is a patch to guarantee that alloc_bootmem_low() allocate DMA area.

Current alloc_bootmem_low() is just specify "goal=0". And it is 
used for __alloc_bootmem_core() to decide which address is better.
However, there is no guarantee that __alloc_bootmem_core()
allocate DMA area when goal=0 is specified.
Even if there is no DMA'ble area in searching node, it allocates
higher address than MAX_DMA_ADDRESS.

__alloc_bootmem_core() is called by order of for_each_pgdat()
in __alloc_bootmem(). So, if first node (node_id = 0) has
DMA'ble area, no trouble will occur. However, our new Itanium2 server
can change which node has lower address. And panic really occurred on it.
The message was "bounce buffer is not DMA'ble" in swiothl_map_single().

To avoid this panic, following patch skips no DMA'ble node when 
lower address is required.
I tested this patch on my Tiger 4 and our new server.

Please apply.

Thanks.

Signed-off by Yasunori Goto <y-goto@jp.fujitsu.com>

Index: allocbootmem/mm/bootmem.c
===================================================================
--- allocbootmem.orig/mm/bootmem.c	2005-06-30 11:57:13.000000000 +0900
+++ allocbootmem/mm/bootmem.c	2005-07-08 20:46:56.209040741 +0900
@@ -387,10 +387,16 @@
 	pg_data_t *pgdat = pgdat_list;
 	void *ptr;
 
-	for_each_pgdat(pgdat)
+	for_each_pgdat(pgdat){
+
+		if (goal < __pa(MAX_DMA_ADDRESS) &&
+		    pgdat->bdata->node_boot_start >= __pa(MAX_DMA_ADDRESS))
+			continue; /* Skip No DMA node */
+
 		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
 						align, goal)))
 			return(ptr);
+	}
 
 	/*
 	 * Whoops, we cannot satisfy the allocation request.

-- 
Yasunori Goto 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Martin J. Bligh]

> This is a patch to guarantee that alloc_bootmem_low() allocate DMA area.
> 
> Current alloc_bootmem_low() is just specify "goal=0". And it is 
> used for __alloc_bootmem_core() to decide which address is better.
> However, there is no guarantee that __alloc_bootmem_core()
> allocate DMA area when goal=0 is specified.
> Even if there is no DMA'ble area in searching node, it allocates
> higher address than MAX_DMA_ADDRESS.
> 
> __alloc_bootmem_core() is called by order of for_each_pgdat()
> in __alloc_bootmem(). So, if first node (node_id = 0) has
> DMA'ble area, no trouble will occur. However, our new Itanium2 server
> can change which node has lower address. And panic really occurred on it.
> The message was "bounce buffer is not DMA'ble" in swiothl_map_single().
> 
> To avoid this panic, following patch skips no DMA'ble node when 
> lower address is required.
> I tested this patch on my Tiger 4 and our new server.

Seems reasonable ... but do you not want to check that the returned
ptr is actually less than MAX_DMA_ADDRESS as well? 

> Please apply.
> 
> Thanks.
> 
> Signed-off by Yasunori Goto <y-goto@jp.fujitsu.com>
> 
> Index: allocbootmem/mm/bootmem.c
> ===================================================================
> --- allocbootmem.orig/mm/bootmem.c	2005-06-30 11:57:13.000000000 +0900
> +++ allocbootmem/mm/bootmem.c	2005-07-08 20:46:56.209040741 +0900
> @@ -387,10 +387,16 @@
>  	pg_data_t *pgdat = pgdat_list;
>  	void *ptr;
>  
> -	for_each_pgdat(pgdat)
> +	for_each_pgdat(pgdat){
> +
> +		if (goal < __pa(MAX_DMA_ADDRESS) &&
> +		    pgdat->bdata->node_boot_start >= __pa(MAX_DMA_ADDRESS))
> +			continue; /* Skip No DMA node */
> +
>  		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
>  						align, goal)))
>  			return(ptr);
> +	}
>  
>  	/*
>  	 * Whoops, we cannot satisfy the allocation request.
> 
> -- 
> Yasunori Goto 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>
> 
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Yasunori Goto]

> > To avoid this panic, following patch skips no DMA'ble node when 
> > lower address is required.
> > I tested this patch on my Tiger 4 and our new server.
> 
> Seems reasonable ... but do you not want to check that the returned
> ptr is actually less than MAX_DMA_ADDRESS as well? 

Well... If there isn't enough DMA area in a node by too much
lower memory request or by something strange memory map in the node,
its case might occur.
I don't know it will really happen. But, the after check might be better
than nothing.

To tell the truth, I did the after check at first
"instead of" previous check like this patch. 
In its patch, if its DMA check failed, 
allocated area are should be freed by free_bootmem_core(). 
But hung up occurred by it, and I changed my patch to previous check
instead of deep investigation of its hung up.

Ok. I'll investigate more.

Thanks.
-- 
Yasunori Goto 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Mike Kravetz]

On Tue, Jul 12, 2005 at 03:50:09PM +0900, Yasunori Goto wrote:
> Index: allocbootmem/mm/bootmem.c
> ===================================================================
> --- allocbootmem.orig/mm/bootmem.c	2005-06-30 11:57:13.000000000 +0900
> +++ allocbootmem/mm/bootmem.c	2005-07-08 20:46:56.209040741 +0900
> @@ -387,10 +387,16 @@
>  	pg_data_t *pgdat = pgdat_list;
>  	void *ptr;
>  
> -	for_each_pgdat(pgdat)
> +	for_each_pgdat(pgdat){
> +
> +		if (goal < __pa(MAX_DMA_ADDRESS) &&
> +		    pgdat->bdata->node_boot_start >= __pa(MAX_DMA_ADDRESS))
> +			continue; /* Skip No DMA node */
> +
>  		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
>  						align, goal)))
>  			return(ptr);
> +	}
>  
>  	/*
>  	 * Whoops, we cannot satisfy the allocation request.

Need to be careful about the use of MAX_DMA_ADDRESS.  It is not always
the case that archs define MAX_DMA_ADDRESS as a real address.  In some
cases, MAX_DMA_ADDRESS is defined as something like -1 to indicate that
all addresses are available for DMA.  I'm not sure that the above code
will always work as desired in such cases.

FYI - While hacking on the memory hotplug code, I added a special
'#define MAX_DMA_PHYSADDR' to get around this issue on such architectures.
Most likely, this isn't elegant enough as a real solution.  But it does
point out that __pa(MAX_DMA_ADDRESS) doesn't always give you what you
expect.

-- 
Mike
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Dave Hansen]

On Tue, 2005-07-12 at 11:30 -0700, Mike Kravetz wrote:
> FYI - While hacking on the memory hotplug code, I added a special
> '#define MAX_DMA_PHYSADDR' to get around this issue on such architectures.
> Most likely, this isn't elegant enough as a real solution.  But it does
> point out that __pa(MAX_DMA_ADDRESS) doesn't always give you what you
> expect.

Didn't we create a MAX_DMA_PHYSADDR or something, so that people could
do this if they want?

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Mike Kravetz]

On Tue, Jul 12, 2005 at 12:29:25PM -0700, Dave Hansen wrote:
> On Tue, 2005-07-12 at 11:30 -0700, Mike Kravetz wrote:
> > FYI - While hacking on the memory hotplug code, I added a special
> > '#define MAX_DMA_PHYSADDR' to get around this issue on such architectures.
> > Most likely, this isn't elegant enough as a real solution.  But it does
> > point out that __pa(MAX_DMA_ADDRESS) doesn't always give you what you
> > expect.
> 
> Didn't we create a MAX_DMA_PHYSADDR or something, so that people could
> do this if they want?

Yes, but that only 'exists' in the hotplug patch set.

My point was simply that __pa(MAX_DMA_ADDRESS) doesn't give you what
you want on all archs.  This patch could add something like MAX_DMA_ADDRESS
to get around the issue.  

I believe that __pa(MAX_DMA_ADDRESS) is also 'incorrectly' used in
the bootmem macros.

#define alloc_bootmem(x) \
        __alloc_bootmem((x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
#define alloc_bootmem_pages(x) \
        __alloc_bootmem((x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))

But, in these cases __pa(MAX_DMA_ADDRESS) is the 'goal' argument.  And
as such, being 'incorrect' is not much of an issue.  Especially on archs
that can do DMA anywhere.

-- 
Mike
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Yasunori Goto]

> On Tue, Jul 12, 2005 at 03:50:09PM +0900, Yasunori Goto wrote:
> > Index: allocbootmem/mm/bootmem.c
> > ===================================================================
> > --- allocbootmem.orig/mm/bootmem.c	2005-06-30 11:57:13.000000000 +0900
> > +++ allocbootmem/mm/bootmem.c	2005-07-08 20:46:56.209040741 +0900
> > @@ -387,10 +387,16 @@
> >  	pg_data_t *pgdat = pgdat_list;
> >  	void *ptr;
> >  
> > -	for_each_pgdat(pgdat)
> > +	for_each_pgdat(pgdat){
> > +
> > +		if (goal < __pa(MAX_DMA_ADDRESS) &&
> > +		    pgdat->bdata->node_boot_start >= __pa(MAX_DMA_ADDRESS))
> > +			continue; /* Skip No DMA node */
> > +
> >  		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
> >  						align, goal)))
> >  			return(ptr);
> > +	}
> >  
> >  	/*
> >  	 * Whoops, we cannot satisfy the allocation request.
> 
> Need to be careful about the use of MAX_DMA_ADDRESS.  It is not always
> the case that archs define MAX_DMA_ADDRESS as a real address.  In some
> cases, MAX_DMA_ADDRESS is defined as something like -1 to indicate that
> all addresses are available for DMA.  I'm not sure that the above code
> will always work as desired in such cases.

Hmm... Thanks for your advise.

If MAX_DMA_ADDRESS is like -1, then all of memory can be DMA'ble, 
right?  How is like this? One more comparison is added.

	if (MAX_DMA_ADDRESS != ~0UL  &&
		goal < __pa(MAX_DMA_ADDRESS) &&
		pgdat->bdata->node_boot_start >= 
		__pa(MAX_DMA_ADDRESS))

Thanks.

-- 
Yasunori Goto 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Mike Kravetz]

On Wed, Jul 13, 2005 at 03:34:48PM +0900, Yasunori Goto wrote:
> If MAX_DMA_ADDRESS is like -1, then all of memory can be DMA'ble, 
> right?  How is like this? One more comparison is added.
> 
> 	if (MAX_DMA_ADDRESS != ~0UL  &&
> 		goal < __pa(MAX_DMA_ADDRESS) &&
> 		pgdat->bdata->node_boot_start >= 
> 		__pa(MAX_DMA_ADDRESS))
> 

I was thinking more about something like the following to eliminate
all the users of __pa(MAX_DMA_ADDRESS).  This patch is NOT complete
as I didn't change arch dependent code using __pa(MAX_DMA_ADDRESS).

Just curious if people think this is overkill, or is there a better
way to address this?

-- 
Mike

diff -Naupr linux-2.6.13-rc2-mm2/include/linux/bootmem.h linux-2.6.13-rc2-mm2.work/include/linux/bootmem.h
--- linux-2.6.13-rc2-mm2/include/linux/bootmem.h	2005-07-06 03:46:33.000000000 +0000
+++ linux-2.6.13-rc2-mm2.work/include/linux/bootmem.h	2005-07-13 21:14:14.000000000 +0000
@@ -40,6 +40,14 @@ typedef struct bootmem_data {
 					 * up searching */
 } bootmem_data_t;
 
+#ifndef MAX_DMA_PHYSADDR
+#if MAX_DMA_ADDRESS == ~0UL
+#define MAX_DMA_PHYSADDR MAX_DMA_ADDRESS
+#else
+#define MAX_DMA_PHYSADDR (__pa(MAX_DMA_ADDRESS))
+#endif
+#endif
+
 extern unsigned long __init bootmem_bootmap_pages (unsigned long);
 extern unsigned long __init init_bootmem (unsigned long addr, unsigned long memend);
 extern void __init free_bootmem (unsigned long addr, unsigned long size);
@@ -47,11 +55,11 @@ extern void * __init __alloc_bootmem (un
 #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
 extern void __init reserve_bootmem (unsigned long addr, unsigned long size);
 #define alloc_bootmem(x) \
-	__alloc_bootmem((x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
+	__alloc_bootmem((x), SMP_CACHE_BYTES, MAX_DMA_PHYSADDR)
 #define alloc_bootmem_low(x) \
 	__alloc_bootmem((x), SMP_CACHE_BYTES, 0)
 #define alloc_bootmem_pages(x) \
-	__alloc_bootmem((x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
+	__alloc_bootmem((x), PAGE_SIZE, MAX_DMA_PHYSADDR)
 #define alloc_bootmem_low_pages(x) \
 	__alloc_bootmem((x), PAGE_SIZE, 0)
 #endif /* !CONFIG_HAVE_ARCH_BOOTMEM_NODE */
@@ -64,9 +72,9 @@ extern unsigned long __init free_all_boo
 extern void * __init __alloc_bootmem_node (pg_data_t *pgdat, unsigned long size, unsigned long align, unsigned long goal);
 #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
 #define alloc_bootmem_node(pgdat, x) \
-	__alloc_bootmem_node((pgdat), (x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
+	__alloc_bootmem_node((pgdat), (x), SMP_CACHE_BYTES, MAX_DMA_PHYSADDR)
 #define alloc_bootmem_pages_node(pgdat, x) \
-	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
+	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, MAX_DMA_PHYSADDR)
 #define alloc_bootmem_low_pages_node(pgdat, x) \
 	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, 0)
 #endif /* !CONFIG_HAVE_ARCH_BOOTMEM_NODE */
diff -Naupr linux-2.6.13-rc2-mm2/mm/bootmem.c linux-2.6.13-rc2-mm2.work/mm/bootmem.c
--- linux-2.6.13-rc2-mm2/mm/bootmem.c	2005-07-06 03:46:33.000000000 +0000
+++ linux-2.6.13-rc2-mm2.work/mm/bootmem.c	2005-07-13 21:18:40.000000000 +0000
@@ -387,10 +387,16 @@ void * __init __alloc_bootmem (unsigned 
 	pg_data_t *pgdat = pgdat_list;
 	void *ptr;
 
-	for_each_pgdat(pgdat)
+	for_each_pgdat(pgdat){
+
+		if (goal < MAX_DMA_PHYSADDR &&
+		    pgdat->bdata->node_boot_start >= MAX_DMA_PHYSADDR)
+			continue; /* Skip No DMA node */
+
 		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
 						align, goal)))
 			return(ptr);
+	}
 
 	/*
 	 * Whoops, we cannot satisfy the allocation request.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH] gurantee DMA area for alloc_bootmem_low()

[Yasunori Goto]

> I was thinking more about something like the following to eliminate
> all the users of __pa(MAX_DMA_ADDRESS).  This patch is NOT complete
> as I didn't change arch dependent code using __pa(MAX_DMA_ADDRESS).
> 
> Just curious if people think this is overkill, or is there a better
> way to address this?

There is no objection at present, I suppose this patch is 
the best way. :)

Thanks.

> diff -Naupr linux-2.6.13-rc2-mm2/include/linux/bootmem.h linux-2.6.13-rc2-mm2.work/include/linux/bootmem.h
> --- linux-2.6.13-rc2-mm2/include/linux/bootmem.h	2005-07-06 03:46:33.000000000 +0000
> +++ linux-2.6.13-rc2-mm2.work/include/linux/bootmem.h	2005-07-13 21:14:14.000000000 +0000
> @@ -40,6 +40,14 @@ typedef struct bootmem_data {
>  					 * up searching */
>  } bootmem_data_t;
>  
> +#ifndef MAX_DMA_PHYSADDR
> +#if MAX_DMA_ADDRESS == ~0UL
> +#define MAX_DMA_PHYSADDR MAX_DMA_ADDRESS
> +#else
> +#define MAX_DMA_PHYSADDR (__pa(MAX_DMA_ADDRESS))
> +#endif
> +#endif
> +
>  extern unsigned long __init bootmem_bootmap_pages (unsigned long);
>  extern unsigned long __init init_bootmem (unsigned long addr, unsigned long memend);
>  extern void __init free_bootmem (unsigned long addr, unsigned long size);
> @@ -47,11 +55,11 @@ extern void * __init __alloc_bootmem (un
>  #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
>  extern void __init reserve_bootmem (unsigned long addr, unsigned long size);
>  #define alloc_bootmem(x) \
> -	__alloc_bootmem((x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
> +	__alloc_bootmem((x), SMP_CACHE_BYTES, MAX_DMA_PHYSADDR)
>  #define alloc_bootmem_low(x) \
>  	__alloc_bootmem((x), SMP_CACHE_BYTES, 0)
>  #define alloc_bootmem_pages(x) \
> -	__alloc_bootmem((x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
> +	__alloc_bootmem((x), PAGE_SIZE, MAX_DMA_PHYSADDR)
>  #define alloc_bootmem_low_pages(x) \
>  	__alloc_bootmem((x), PAGE_SIZE, 0)
>  #endif /* !CONFIG_HAVE_ARCH_BOOTMEM_NODE */
> @@ -64,9 +72,9 @@ extern unsigned long __init free_all_boo
>  extern void * __init __alloc_bootmem_node (pg_data_t *pgdat, unsigned long size, unsigned long align, unsigned long goal);
>  #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
>  #define alloc_bootmem_node(pgdat, x) \
> -	__alloc_bootmem_node((pgdat), (x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
> +	__alloc_bootmem_node((pgdat), (x), SMP_CACHE_BYTES, MAX_DMA_PHYSADDR)
>  #define alloc_bootmem_pages_node(pgdat, x) \
> -	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
> +	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, MAX_DMA_PHYSADDR)
>  #define alloc_bootmem_low_pages_node(pgdat, x) \
>  	__alloc_bootmem_node((pgdat), (x), PAGE_SIZE, 0)
>  #endif /* !CONFIG_HAVE_ARCH_BOOTMEM_NODE */
> diff -Naupr linux-2.6.13-rc2-mm2/mm/bootmem.c linux-2.6.13-rc2-mm2.work/mm/bootmem.c
> --- linux-2.6.13-rc2-mm2/mm/bootmem.c	2005-07-06 03:46:33.000000000 +0000
> +++ linux-2.6.13-rc2-mm2.work/mm/bootmem.c	2005-07-13 21:18:40.000000000 +0000
> @@ -387,10 +387,16 @@ void * __init __alloc_bootmem (unsigned 
>  	pg_data_t *pgdat = pgdat_list;
>  	void *ptr;
>  
> -	for_each_pgdat(pgdat)
> +	for_each_pgdat(pgdat){
> +
> +		if (goal < MAX_DMA_PHYSADDR &&
> +		    pgdat->bdata->node_boot_start >= MAX_DMA_PHYSADDR)
> +			continue; /* Skip No DMA node */
> +
>  		if ((ptr = __alloc_bootmem_core(pgdat->bdata, size,
>  						align, goal)))
>  			return(ptr);
> +	}
>  
>  	/*
>  	 * Whoops, we cannot satisfy the allocation request.

-- 
Yasunori Goto 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>