Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Johannes Weiner]

On Tue, Feb 24, 2009 at 08:23:03PM +0000, Tejun Heo wrote:
> Author:     Tejun Heo <tj@kernel.org>
> AuthorDate: Tue, 24 Feb 2009 11:57:20 +0900
> Commit:     Tejun Heo <tj@kernel.org>
> CommitDate: Tue, 24 Feb 2009 11:57:20 +0900
> 
> bootmem: clean up arch-specific bootmem wrapping
> 
> Impact: cleaner and consistent bootmem wrapping
> 
> By setting CONFIG_HAVE_ARCH_BOOTMEM_NODE, archs can define
> arch-specific wrappers for bootmem allocation.  However, this is done
> a bit strangely in that only the high level convenience macros can be
> changed while lower level, but still exported, interface functions
> can't be wrapped.  This not only is messy but also leads to strange
> situation where alloc_bootmem() does what the arch wants it to do but
> the equivalent __alloc_bootmem() call doesn't although they should be
> able to be used interchangeably.
> 
> This patch updates bootmem such that archs can override / wrap the
> backend function - alloc_bootmem_core() instead of the highlevel
> interface functions to allow simpler and consistent wrapping.  Also,
> HAVE_ARCH_BOOTMEM_NODE is renamed to HAVE_ARCH_BOOTMEM.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Johannes Weiner <hannes@saeurebad.de>

What does this message mean?  That the patch was commited to the -tip
tree?

Well, why not... oh, right, it is broken ;-)

> ---
>  arch/avr32/Kconfig               |    2 +-
>  arch/x86/Kconfig                 |    2 +-
>  arch/x86/include/asm/mmzone_32.h |   43 ++++---------------------------------
>  include/linux/bootmem.h          |   10 +++-----
>  mm/bootmem.c                     |   14 +++++++++--
>  5 files changed, 22 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/avr32/Kconfig b/arch/avr32/Kconfig
> index b189680..05fe305 100644
> --- a/arch/avr32/Kconfig
> +++ b/arch/avr32/Kconfig
> @@ -181,7 +181,7 @@ source "kernel/Kconfig.preempt"
>  config QUICKLIST
>  	def_bool y
>  
> -config HAVE_ARCH_BOOTMEM_NODE
> +config HAVE_ARCH_BOOTMEM
>  	def_bool n
>  
>  config ARCH_HAVE_MEMORY_PRESENT
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d3f6ead..6fd3b23 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1111,7 +1111,7 @@ config NODES_SHIFT
>  	  Specify the maximum number of NUMA Nodes available on the target
>  	  system.  Increases memory reserved to accomodate various tables.
>  
> -config HAVE_ARCH_BOOTMEM_NODE
> +config HAVE_ARCH_BOOTMEM
>  	def_bool y
>  	depends on X86_32 && NUMA
>  
> diff --git a/arch/x86/include/asm/mmzone_32.h b/arch/x86/include/asm/mmzone_32.h
> index 07f1af4..1e0fa9e 100644
> --- a/arch/x86/include/asm/mmzone_32.h
> +++ b/arch/x86/include/asm/mmzone_32.h
> @@ -93,45 +93,12 @@ static inline int pfn_valid(int pfn)
>  #endif /* CONFIG_DISCONTIGMEM */
>  
>  #ifdef CONFIG_NEED_MULTIPLE_NODES
> -
> -/*
> - * Following are macros that are specific to this numa platform.
> - */
> -#define reserve_bootmem(addr, size, flags) \
> -	reserve_bootmem_node(NODE_DATA(0), (addr), (size), (flags))

With the removal of the #ifdef around the generic reserve_bootmem(),
this will no longer use just node 0, but all numa nodes addr -
addr+size spans.

If it doesn't matter, well, okay.  But the patch description doesn't
say anything about this change.

> -#define alloc_bootmem(x) \
> -	__alloc_bootmem_node(NODE_DATA(0), (x), SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
> -#define alloc_bootmem_nopanic(x) \
> -	__alloc_bootmem_node_nopanic(NODE_DATA(0), (x), SMP_CACHE_BYTES, \
> -				__pa(MAX_DMA_ADDRESS))
> -#define alloc_bootmem_low(x) \
> -	__alloc_bootmem_node(NODE_DATA(0), (x), SMP_CACHE_BYTES, 0)
> -#define alloc_bootmem_pages(x) \
> -	__alloc_bootmem_node(NODE_DATA(0), (x), PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
> -#define alloc_bootmem_pages_nopanic(x) \
> -	__alloc_bootmem_node_nopanic(NODE_DATA(0), (x), PAGE_SIZE, \
> -				__pa(MAX_DMA_ADDRESS))
> -#define alloc_bootmem_low_pages(x) \
> -	__alloc_bootmem_node(NODE_DATA(0), (x), PAGE_SIZE, 0)
> -#define alloc_bootmem_node(pgdat, x)					\
> -({									\
> -	struct pglist_data  __maybe_unused			\
> -				*__alloc_bootmem_node__pgdat = (pgdat);	\
> -	__alloc_bootmem_node(NODE_DATA(0), (x), SMP_CACHE_BYTES,	\
> -						__pa(MAX_DMA_ADDRESS));	\
> -})
> -#define alloc_bootmem_pages_node(pgdat, x)				\
> -({									\
> -	struct pglist_data  __maybe_unused			\
> -				*__alloc_bootmem_node__pgdat = (pgdat);	\
> -	__alloc_bootmem_node(NODE_DATA(0), (x), PAGE_SIZE,		\
> -						__pa(MAX_DMA_ADDRESS));	\
> -})
> -#define alloc_bootmem_low_pages_node(pgdat, x)				\
> +/* always use node 0 for bootmem on this numa platform */
> +#define alloc_bootmem_core(__bdata, size, align, goal, limit)		\
>  ({									\
> -	struct pglist_data  __maybe_unused			\
> -				*__alloc_bootmem_node__pgdat = (pgdat);	\
> -	__alloc_bootmem_node(NODE_DATA(0), (x), PAGE_SIZE, 0);		\
> +	bootmem_data_t __maybe_unused *	__abm_bdata_dummy = (__bdata);	\
> +	__alloc_bootmem_core(NODE_DATA(0)->bdata,			\
> +			     (size), (align), (goal), (limit));		\
>  })
>  #endif /* CONFIG_NEED_MULTIPLE_NODES */
>  
> diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> index 95837bf..3a87f93 100644
> --- a/include/linux/bootmem.h
> +++ b/include/linux/bootmem.h
> @@ -69,10 +69,9 @@ extern int reserve_bootmem_node(pg_data_t *pgdat,
>  				 unsigned long physaddr,
>  				 unsigned long size,
>  				 int flags);
> -#ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
> -extern int reserve_bootmem(unsigned long addr, unsigned long size, int flags);
> -#endif
> -
> +extern int reserve_bootmem(unsigned long addr,
> +			   unsigned long size,
> +			   int flags);
>  extern void *__alloc_bootmem_nopanic(unsigned long size,
>  			     unsigned long align,
>  			     unsigned long goal);
> @@ -94,7 +93,7 @@ extern void *__alloc_bootmem_low_node(pg_data_t *pgdat,
>  				      unsigned long size,
>  				      unsigned long align,
>  				      unsigned long goal);
> -#ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
> +
>  #define alloc_bootmem(x) \
>  	__alloc_bootmem(x, SMP_CACHE_BYTES, __pa(MAX_DMA_ADDRESS))
>  #define alloc_bootmem_nopanic(x) \
> @@ -113,7 +112,6 @@ extern void *__alloc_bootmem_low_node(pg_data_t *pgdat,
>  	__alloc_bootmem_node(pgdat, x, PAGE_SIZE, __pa(MAX_DMA_ADDRESS))
>  #define alloc_bootmem_low_pages_node(pgdat, x) \
>  	__alloc_bootmem_low_node(pgdat, x, PAGE_SIZE, 0)
> -#endif /* !CONFIG_HAVE_ARCH_BOOTMEM_NODE */
>  
>  extern int reserve_bootmem_generic(unsigned long addr, unsigned long size,
>  				   int flags);
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 51a0ccf..d7140c0 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -37,6 +37,16 @@ static struct list_head bdata_list __initdata = LIST_HEAD_INIT(bdata_list);
>  
>  static int bootmem_debug;
>  
> +/*
> + * If an arch needs to apply workarounds to bootmem allocation, it can
> + * set CONFIG_HAVE_ARCH_BOOTMEM and define a wrapper around
> + * __alloc_bootmem_core().
> + */
> +#ifndef CONFIG_HAVE_ARCH_BOOTMEM
> +#define alloc_bootmem_core(bdata, size, align, goal, limit)		\
> +	__alloc_bootmem_core((bdata), (size), (align), (goal), (limit))
> +#endif

Since it only allows macros anyway (__alloc_bootmem_core is local to
bootmem.c), we could just use this in asm/mmzone_32.h:

	#define alloc_bootmem_core(__bdata, size, align, goal, limit)	\
	({								\
		...							\
		alloc_bootmem_core(...);				\
	})

This should work without even renaming alloc_bootmem_core internally,
no?

Or would it be even better to export __alloc_bootmem_core() and allow
static inline wrappers that do nice type checking as well?

	Hannes

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Ingo Molnar]

* Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Tue, Feb 24, 2009 at 08:23:03PM +0000, Tejun Heo wrote:
> > Author:     Tejun Heo <tj@kernel.org>
> > AuthorDate: Tue, 24 Feb 2009 11:57:20 +0900
> > Commit:     Tejun Heo <tj@kernel.org>
> > CommitDate: Tue, 24 Feb 2009 11:57:20 +0900
> > 
> > bootmem: clean up arch-specific bootmem wrapping
> > 
> > Impact: cleaner and consistent bootmem wrapping
> > 
> > By setting CONFIG_HAVE_ARCH_BOOTMEM_NODE, archs can define
> > arch-specific wrappers for bootmem allocation.  However, this is done
> > a bit strangely in that only the high level convenience macros can be
> > changed while lower level, but still exported, interface functions
> > can't be wrapped.  This not only is messy but also leads to strange
> > situation where alloc_bootmem() does what the arch wants it to do but
> > the equivalent __alloc_bootmem() call doesn't although they should be
> > able to be used interchangeably.
> > 
> > This patch updates bootmem such that archs can override / wrap the
> > backend function - alloc_bootmem_core() instead of the highlevel
> > interface functions to allow simpler and consistent wrapping.  Also,
> > HAVE_ARCH_BOOTMEM_NODE is renamed to HAVE_ARCH_BOOTMEM.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Johannes Weiner <hannes@saeurebad.de>
> 
> What does this message mean?  That the patch was commited to 
> the -tip tree?

yes.

> Well, why not... oh, right, it is broken ;-)

In your reply you pointed out a change that was not adequately 
declared plus an opportunity for a cleanup - is that what you 
mean by breakage?

	Ingo

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Johannes Weiner]

On Tue, Feb 24, 2009 at 10:49:32PM +0100, Ingo Molnar wrote:
> 
> * Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > On Tue, Feb 24, 2009 at 08:23:03PM +0000, Tejun Heo wrote:
> > > Author:     Tejun Heo <tj@kernel.org>
> > > AuthorDate: Tue, 24 Feb 2009 11:57:20 +0900
> > > Commit:     Tejun Heo <tj@kernel.org>
> > > CommitDate: Tue, 24 Feb 2009 11:57:20 +0900
> > > 
> > > bootmem: clean up arch-specific bootmem wrapping
> > > 
> > > Impact: cleaner and consistent bootmem wrapping
> > > 
> > > By setting CONFIG_HAVE_ARCH_BOOTMEM_NODE, archs can define
> > > arch-specific wrappers for bootmem allocation.  However, this is done
> > > a bit strangely in that only the high level convenience macros can be
> > > changed while lower level, but still exported, interface functions
> > > can't be wrapped.  This not only is messy but also leads to strange
> > > situation where alloc_bootmem() does what the arch wants it to do but
> > > the equivalent __alloc_bootmem() call doesn't although they should be
> > > able to be used interchangeably.
> > > 
> > > This patch updates bootmem such that archs can override / wrap the
> > > backend function - alloc_bootmem_core() instead of the highlevel
> > > interface functions to allow simpler and consistent wrapping.  Also,
> > > HAVE_ARCH_BOOTMEM_NODE is renamed to HAVE_ARCH_BOOTMEM.
> > > 
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Cc: Johannes Weiner <hannes@saeurebad.de>
> > 
> > What does this message mean?  That the patch was commited to 
> > the -tip tree?
> 
> yes.
> 
> > Well, why not... oh, right, it is broken ;-)
> 
> In your reply you pointed out a change that was not adequately 
> declared plus an opportunity for a cleanup - is that what you 
> mean by breakage?

No, when the patch was submitted for review, I pointed out the change
in semantics and gathered from Tejun's reaction that this wasn't done
intentionally.  So the problem is the change itself, not the missing
declaration.

>From the original mail:

	Johannes Weiner wrote:
	> This won't suffice as reserve_bootmem() doesn't use
	> alloc_bootmem_core(), so now you effectively removed the
	> node-0 restriction for reserve_bootmem() on this
	> configuration.

	Ah... right.  :-(

I just wrote again because I didn't understand why Tejun acknowledged
the error in the patch and then it went into -tip anyway.

The other part of my email was just suggestions for a cleanup, I
wasn't referring to that when I said 'broken' - sorry if that is how
it came over.

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Tejun Heo]

Hello,

Johannes Weiner wrote:
> No, when the patch was submitted for review, I pointed out the change
> in semantics and gathered from Tejun's reaction that this wasn't done
> intentionally.  So the problem is the change itself, not the missing
> declaration.

Yeah, I should have regenerated the tree.  Sorry about that.

>>From the original mail:
> 
> 	Johannes Weiner wrote:
> 	> This won't suffice as reserve_bootmem() doesn't use
> 	> alloc_bootmem_core(), so now you effectively removed the
> 	> node-0 restriction for reserve_bootmem() on this
> 	> configuration.
> 
> 	Ah... right.  :-(
> 
> I just wrote again because I didn't understand why Tejun acknowledged
> the error in the patch and then it went into -tip anyway.
> 
> The other part of my email was just suggestions for a cleanup, I
> wasn't referring to that when I said 'broken' - sorry if that is how
> it came over.

It seems that the wrapping thing was broken both before and after the
patch and can lead to panic on free path.  I'll soon post a patch to
fix it.

Thanks.

-- 
tejun

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Johannes Weiner]

On Wed, Feb 25, 2009 at 11:27:58AM +0900, Tejun Heo wrote:
> Hello,
> 
> Johannes Weiner wrote:
> > No, when the patch was submitted for review, I pointed out the change
> > in semantics and gathered from Tejun's reaction that this wasn't done
> > intentionally.  So the problem is the change itself, not the missing
> > declaration.
> 
> Yeah, I should have regenerated the tree.  Sorry about that.

Sorry about my rude way of commenting.

> >>>From the original mail:
> > 
> > 	Johannes Weiner wrote:
> > 	> This won't suffice as reserve_bootmem() doesn't use
> > 	> alloc_bootmem_core(), so now you effectively removed the
> > 	> node-0 restriction for reserve_bootmem() on this
> > 	> configuration.
> > 
> > 	Ah... right.  :-(
> > 
> > I just wrote again because I didn't understand why Tejun acknowledged
> > the error in the patch and then it went into -tip anyway.
> > 
> > The other part of my email was just suggestions for a cleanup, I
> > wasn't referring to that when I said 'broken' - sorry if that is how
> > it came over.
> 
> It seems that the wrapping thing was broken both before and after the
> patch and can lead to panic on free path.  I'll soon post a patch to
> fix it.

Ok, thanks for looking into it!

	Hannes

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Tejun Heo]

Commit c132937556f56ee4b831ef4b23f1846e05fde102 tried to clean up
bootmem arch wrapper but it wasn't quite correct.  Before the commit,
the followings were broken.

* Low level interface functions prefixed with __ ignored arch
  preference.

* reserve_bootmem(...) can't be mapped into
  reserve_bootmem_node(NODE_DATA(0)->bdata, ...) because the node is
  not preference here.  The region specified MUST fall into the
  specified region; otherwise, it will panic.

After the commit,

* If allocation fails for the arch preferred node, it should fallback
  to whatever is available.  Instead, it simply failed allocation.

There are too many internal details to allow generic wrapping and
still keep things simple for archs.  Plus, all that arch wants is a
way to prefer certain node over another.

This patch drops the generic wrapping around alloc_bootmem_core() and
add alloc_bootmem_core() instead.  If necessary, arch can define
bootmem_arch_referred_node() macro or function which takes all
allocation information and returns the preferred node.  bootmem
generic code will always try the preferred node first and then
fallback to other nodes as usual.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
It turns out reserve_bootmem() shouldn't be wrapped in the first
place.  I hope I got it right this time.  Can you please review this
one?  And I didn't think your comment was rude at all, no worries.

Thanks.

 arch/x86/include/asm/mmzone_32.h |    8 +-----
 mm/bootmem.c                     |   45 ++++++++++++++++++++++++++-------------
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/mmzone_32.h b/arch/x86/include/asm/mmzone_32.h
index eeacf67..ede6998 100644
--- a/arch/x86/include/asm/mmzone_32.h
+++ b/arch/x86/include/asm/mmzone_32.h
@@ -92,12 +92,8 @@ static inline int pfn_valid(int pfn)
 
 #ifdef CONFIG_NEED_MULTIPLE_NODES
 /* always use node 0 for bootmem on this numa platform */
-#define alloc_bootmem_core(__bdata, size, align, goal, limit)		\
-({									\
-	bootmem_data_t __maybe_unused *	__abm_bdata_dummy = (__bdata);	\
-	__alloc_bootmem_core(NODE_DATA(0)->bdata,			\
-			     (size), (align), (goal), (limit));		\
-})
+#define bootmem_arch_preferred_node(__bdata, size, align, goal, limit)	\
+	(NODE_DATA(0)->bdata)
 #endif /* CONFIG_NEED_MULTIPLE_NODES */
 
 #endif /* _ASM_X86_MMZONE_32_H */
diff --git a/mm/bootmem.c b/mm/bootmem.c
index d7140c0..daf9271 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -37,16 +37,6 @@ static struct list_head bdata_list __initdata = LIST_HEAD_INIT(bdata_list);
 
 static int bootmem_debug;
 
-/*
- * If an arch needs to apply workarounds to bootmem allocation, it can
- * set CONFIG_HAVE_ARCH_BOOTMEM and define a wrapper around
- * __alloc_bootmem_core().
- */
-#ifndef CONFIG_HAVE_ARCH_BOOTMEM
-#define alloc_bootmem_core(bdata, size, align, goal, limit)		\
-	__alloc_bootmem_core((bdata), (size), (align), (goal), (limit))
-#endif
-
 static int __init bootmem_debug_setup(char *buf)
 {
 	bootmem_debug = 1;
@@ -436,9 +426,9 @@ static unsigned long align_off(struct bootmem_data *bdata, unsigned long off,
 	return ALIGN(base + off, align) - base;
 }
 
-static void * __init __alloc_bootmem_core(struct bootmem_data *bdata,
-				unsigned long size, unsigned long align,
-				unsigned long goal, unsigned long limit)
+static void * __init alloc_bootmem_core(struct bootmem_data *bdata,
+					unsigned long size, unsigned long align,
+					unsigned long goal, unsigned long limit)
 {
 	unsigned long fallback = 0;
 	unsigned long min, max, start, sidx, midx, step;
@@ -538,17 +528,34 @@ find_block:
 	return NULL;
 }
 
+static void * __init alloc_arch_preferred_bootmem(bootmem_data_t *bdata,
+					unsigned long size, unsigned long align,
+					unsigned long goal, unsigned long limit)
+{
+#ifdef CONFIG_HAVE_ARCH_BOOTMEM
+	bootmem_data_t *p_bdata;
+
+	p_bdata = bootmem_arch_preferred_node(bdata, size, align, goal, limit);
+	if (p_bdata)
+		return alloc_bootmem_core(p_bdata, size, align, goal, limit);
+#endif
+	return NULL;
+}
+
 static void * __init ___alloc_bootmem_nopanic(unsigned long size,
 					unsigned long align,
 					unsigned long goal,
 					unsigned long limit)
 {
 	bootmem_data_t *bdata;
+	void *region;
 
 restart:
-	list_for_each_entry(bdata, &bdata_list, list) {
-		void *region;
+	region = alloc_arch_preferred_bootmem(NULL, size, align, goal, limit);
+	if (region)
+		return region;
 
+	list_for_each_entry(bdata, &bdata_list, list) {
 		if (goal && bdata->node_low_pfn <= PFN_DOWN(goal))
 			continue;
 		if (limit && bdata->node_min_pfn >= PFN_DOWN(limit))
@@ -626,6 +633,10 @@ static void * __init ___alloc_bootmem_node(bootmem_data_t *bdata,
 {
 	void *ptr;
 
+	ptr = alloc_arch_preferred_bootmem(bdata, size, align, goal, limit);
+	if (ptr)
+		return ptr;
+
 	ptr = alloc_bootmem_core(bdata, size, align, goal, limit);
 	if (ptr)
 		return ptr;
@@ -682,6 +693,10 @@ void * __init __alloc_bootmem_node_nopanic(pg_data_t *pgdat, unsigned long size,
 {
 	void *ptr;
 
+	ptr = alloc_arch_preferred_bootmem(pgdat->bdata, size, align, goal, 0);
+	if (ptr)
+		return ptr;
+
 	ptr = alloc_bootmem_core(pgdat->bdata, size, align, goal, 0);
 	if (ptr)
 		return ptr;

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Tejun Heo]

Tejun Heo wrote:
> This patch drops the generic wrapping around alloc_bootmem_core() and
> add alloc_bootmem_core() instead.  If necessary, arch can define
      ^^^^^^^^^^^^^^^^^^^^
should have been alloc_arch_preferred_bootmem()

> bootmem_arch_referred_node() macro or function which takes all
> allocation information and returns the preferred node.  bootmem
> generic code will always try the preferred node first and then
> fallback to other nodes as usual.

-- 
tejun

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Tejun Heo]

Tejun Heo wrote:
> Tejun Heo wrote:
>> This patch drops the generic wrapping around alloc_bootmem_core() and
>> add alloc_bootmem_core() instead.  If necessary, arch can define
>       ^^^^^^^^^^^^^^^^^^^^
> should have been alloc_arch_preferred_bootmem()
> 
>> bootmem_arch_referred_node() macro or function which takes all
>> allocation information and returns the preferred node.  bootmem
>> generic code will always try the preferred node first and then
>> fallback to other nodes as usual.

Ping. Johannes, can you please review the patch?

Thanks.

-- 
tejun

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Johannes Weiner]

On Wed, Feb 25, 2009 at 01:52:35PM +0900, Tejun Heo wrote:
> Commit c132937556f56ee4b831ef4b23f1846e05fde102 tried to clean up
> bootmem arch wrapper but it wasn't quite correct.  Before the commit,
> the followings were broken.
> 
> * Low level interface functions prefixed with __ ignored arch
>   preference.
> 
> * reserve_bootmem(...) can't be mapped into
>   reserve_bootmem_node(NODE_DATA(0)->bdata, ...) because the node is
>   not preference here.  The region specified MUST fall into the
>   specified region; otherwise, it will panic.
> 
> After the commit,
> 
> * If allocation fails for the arch preferred node, it should fallback
>   to whatever is available.  Instead, it simply failed allocation.
> 
> There are too many internal details to allow generic wrapping and
> still keep things simple for archs.  Plus, all that arch wants is a
> way to prefer certain node over another.
> 
> This patch drops the generic wrapping around alloc_bootmem_core() and
> add alloc_bootmem_core() instead.  If necessary, arch can define
> bootmem_arch_referred_node() macro or function which takes all
> allocation information and returns the preferred node.  bootmem
> generic code will always try the preferred node first and then
> fallback to other nodes as usual.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
> It turns out reserve_bootmem() shouldn't be wrapped in the first
> place.  I hope I got it right this time.  Can you please review this
> one?  And I didn't think your comment was rude at all, no worries.

You are right!  Sorry, I missed that.  Yes, reserve_bootmem() operates
on ranges disregarding nodes.

>  arch/x86/include/asm/mmzone_32.h |    8 +-----
>  mm/bootmem.c                     |   45 ++++++++++++++++++++++++++-------------
>  2 files changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mmzone_32.h b/arch/x86/include/asm/mmzone_32.h
> index eeacf67..ede6998 100644
> --- a/arch/x86/include/asm/mmzone_32.h
> +++ b/arch/x86/include/asm/mmzone_32.h
> @@ -92,12 +92,8 @@ static inline int pfn_valid(int pfn)
>  
>  #ifdef CONFIG_NEED_MULTIPLE_NODES
>  /* always use node 0 for bootmem on this numa platform */
> -#define alloc_bootmem_core(__bdata, size, align, goal, limit)		\
> -({									\
> -	bootmem_data_t __maybe_unused *	__abm_bdata_dummy = (__bdata);	\
> -	__alloc_bootmem_core(NODE_DATA(0)->bdata,			\
> -			     (size), (align), (goal), (limit));		\
> -})
> +#define bootmem_arch_preferred_node(__bdata, size, align, goal, limit)	\
> +	(NODE_DATA(0)->bdata)
>  #endif /* CONFIG_NEED_MULTIPLE_NODES */
>  
>  #endif /* _ASM_X86_MMZONE_32_H */
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index d7140c0..daf9271 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -37,16 +37,6 @@ static struct list_head bdata_list __initdata = LIST_HEAD_INIT(bdata_list);
>  
>  static int bootmem_debug;
>  
> -/*
> - * If an arch needs to apply workarounds to bootmem allocation, it can
> - * set CONFIG_HAVE_ARCH_BOOTMEM and define a wrapper around
> - * __alloc_bootmem_core().
> - */
> -#ifndef CONFIG_HAVE_ARCH_BOOTMEM
> -#define alloc_bootmem_core(bdata, size, align, goal, limit)		\
> -	__alloc_bootmem_core((bdata), (size), (align), (goal), (limit))
> -#endif
> -
>  static int __init bootmem_debug_setup(char *buf)
>  {
>  	bootmem_debug = 1;
> @@ -436,9 +426,9 @@ static unsigned long align_off(struct bootmem_data *bdata, unsigned long off,
>  	return ALIGN(base + off, align) - base;
>  }
>  
> -static void * __init __alloc_bootmem_core(struct bootmem_data *bdata,
> -				unsigned long size, unsigned long align,
> -				unsigned long goal, unsigned long limit)
> +static void * __init alloc_bootmem_core(struct bootmem_data *bdata,
> +					unsigned long size, unsigned long align,
> +					unsigned long goal, unsigned long limit)
>  {
>  	unsigned long fallback = 0;
>  	unsigned long min, max, start, sidx, midx, step;
> @@ -538,17 +528,34 @@ find_block:
>  	return NULL;
>  }
>  
> +static void * __init alloc_arch_preferred_bootmem(bootmem_data_t *bdata,
> +					unsigned long size, unsigned long align,
> +					unsigned long goal, unsigned long limit)
> +{
> +#ifdef CONFIG_HAVE_ARCH_BOOTMEM
> +	bootmem_data_t *p_bdata;
> +
> +	p_bdata = bootmem_arch_preferred_node(bdata, size, align, goal, limit);
> +	if (p_bdata)
> +		return alloc_bootmem_core(p_bdata, size, align, goal, limit);
> +#endif
> +	return NULL;
> +}
> +
>  static void * __init ___alloc_bootmem_nopanic(unsigned long size,
>  					unsigned long align,
>  					unsigned long goal,
>  					unsigned long limit)
>  {
>  	bootmem_data_t *bdata;
> +	void *region;
>  
>  restart:
> -	list_for_each_entry(bdata, &bdata_list, list) {
> -		void *region;
> +	region = alloc_arch_preferred_bootmem(NULL, size, align, goal, limit);
> +	if (region)
> +		return region;
>  
> +	list_for_each_entry(bdata, &bdata_list, list) {
>  		if (goal && bdata->node_low_pfn <= PFN_DOWN(goal))
>  			continue;
>  		if (limit && bdata->node_min_pfn >= PFN_DOWN(limit))
> @@ -626,6 +633,10 @@ static void * __init ___alloc_bootmem_node(bootmem_data_t *bdata,
>  {
>  	void *ptr;
>  
> +	ptr = alloc_arch_preferred_bootmem(bdata, size, align, goal, limit);
> +	if (ptr)
> +		return ptr;
> +
>  	ptr = alloc_bootmem_core(bdata, size, align, goal, limit);
>  	if (ptr)
>  		return ptr;
> @@ -682,6 +693,10 @@ void * __init __alloc_bootmem_node_nopanic(pg_data_t *pgdat, unsigned long size,
>  {
>  	void *ptr;
>  
> +	ptr = alloc_arch_preferred_bootmem(pgdat->bdata, size, align, goal, 0);
> +	if (ptr)
> +		return ptr;
> +
>  	ptr = alloc_bootmem_core(pgdat->bdata, size, align, goal, 0);
>  	if (ptr)
>  		return ptr;

Well, okay.

It is a bugfix for preferring the arch node even with the __interface.
But it brings ifdeffery/special casing to bootmem that was before in a
header file and is only for a very rare hardware setup, so I don't
think it is a cleanup, rather a necessity.

For now, let's just go with it.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [tip:core/percpu] bootmem: clean up arch-specific bootmem wrapping

[Ingo Molnar]

* Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> Johannes Weiner wrote:
> > No, when the patch was submitted for review, I pointed out the change
> > in semantics and gathered from Tejun's reaction that this wasn't done
> > intentionally.  So the problem is the change itself, not the missing
> > declaration.
> 
> Yeah, I should have regenerated the tree.  Sorry about that.

Queueing up a fix is still worth having of course. That will 
also more fairly document Johannes's review efforts.

	Ingo