Re: [PATCH 2/3] mm/memblock: Allocate boot time data structures from mirrored memory

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 3 Feb 2015 14:38:02 -0800 Tony Luck <tony.luck@intel.com> wrote:

> Try to allocate all boot time kernel data structures from mirrored
> memory. If we run out of mirrored memory print warnings, but fall
> back to using non-mirrored memory to make sure that we still boot.
> 
> ...
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 1d448879caae..20bf3dfab564 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -22,6 +22,7 @@
>  
>  /* Definition of memblock flags. */
>  #define MEMBLOCK_HOTPLUG	0x1	/* hotpluggable region */
> +#define MEMBLOCK_MIRROR		0x2	/* mirrored region */

It would be nice to make these an enum.  Then all those literal "0"'s
which were added in [1/3] become MEMBLOCK_NONE, which is
self-documenting.

>
> ...
>
> +static inline bool memblock_is_mirror(struct memblock_region *m)
> +{
> +	return m->flags & MEMBLOCK_MIRROR;
> +}
> +
>
> ...
>
> +u32 __init_memblock memblock_has_mirror(void)
> +{
> +	return memblock_have_mirror ? MEMBLOCK_MIRROR : 0;
> +}

hm, these are very similar.  But I guess they're different enough.

Gramatically, a function called "memblock_has_mirror()" should return a
bool.  This guy is misnamed.  "memblock_mirror_flag()"?


>  /* inline so we don't get a warning when pr_debug is compiled out */
>  static __init_memblock const char *
>  memblock_type_name(struct memblock_type *type)
> @@ -257,8 +263,19 @@ phys_addr_t __init_memblock memblock_find_in_range(phys_addr_t start,
>  					phys_addr_t end, phys_addr_t size,
>  					phys_addr_t align)
>  {
> -	return memblock_find_in_range_node(size, align, start, end,
> +	phys_addr_t ret;
> +	u32 flag = memblock_has_mirror();
> +
> +	ret = memblock_find_in_range_node(size, align, start, end,
> +					    NUMA_NO_NODE, flag);
> +
> +	if (!ret && flag) {
> +		pr_warn("Could not allocate %lld bytes of mirrored memory\n", size);

This printk will warn on some configs.  Print a phys_addr_t with %pap. 
I think.  See huge comment over lib/vsprintf.c:pointer().  There are
other instances of this.

> +		ret = memblock_find_in_range_node(size, align, start, end,
>  					    NUMA_NO_NODE, 0);
> +	}
> +
> +	return ret;
>  }
>
> ...
>
>  phys_addr_t __init memblock_alloc_nid(phys_addr_t size, phys_addr_t align, int nid)
>  {
> -	return memblock_alloc_base_nid(size, align, MEMBLOCK_ALLOC_ACCESSIBLE, nid, 0);
> +	u32 flag = memblock_has_mirror();
> +	phys_addr_t ret;
> +
> +again:
> +	ret = memblock_alloc_base_nid(size, align, MEMBLOCK_ALLOC_ACCESSIBLE, nid, flag);
> +
> +	if (!ret && flag) {
> +		flag = 0;
> +		goto again;
> +	}

What's going on here?  This is where we're falling back to
non-mirrored.  But it's happening silently?  Should it warn, or is that
handled elsewhere?

This function isn't specific to mirrored memory - for any future flags,
falling back to flags==0 may not be the desired behavior.  What do we
do then?  I guess

	if (!ret && (flag & MEMBLOCK_MIRROR)) (
		flag &= ~MEMBLOCK_MIRROR;
		goto again;

yes?

That can be done later if needed, I suppose.

> +	return ret;
>  }
>  
>
> ...
>
> @@ -1181,13 +1232,13 @@ static void * __init memblock_virt_alloc_internal(
>  
>  again:
>  	alloc = memblock_find_in_range_node(size, align, min_addr, max_addr,
> -					    nid, 0);
> +					    nid, flag);
>  	if (alloc)
>  		goto done;
>  
>  	if (nid != NUMA_NO_NODE) {
>  		alloc = memblock_find_in_range_node(size, align, min_addr,
> -						    max_addr,  NUMA_NO_NODE, 0);
> +						    max_addr,  NUMA_NO_NODE, flag);
>  		if (alloc)
>  			goto done;
>  	}
> @@ -1195,10 +1246,15 @@ again:
>  	if (min_addr) {
>  		min_addr = 0;
>  		goto again;
> -	} else {
> -		goto error;
>  	}
>  
> +	if (flag) {
> +		flag = 0;
> +		pr_warn("Could not allocate %lld bytes of mirrored memory\n", size);

printk warning.

Please don't torture people who use 80-col displays!

> +		goto again;
> +	}
> +
> +	return NULL;
>
> ...
>
> @@ -37,11 +37,19 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align,
>  {
>  	void *ptr;
>  	u64 addr;
> +	u32 flag = memblock_has_mirror();
>  
>  	if (limit > memblock.current_limit)
>  		limit = memblock.current_limit;
>  
> -	addr = memblock_find_in_range_node(size, align, goal, limit, nid, 0);
> +again:
> +	addr = memblock_find_in_range_node(size, align, goal, limit, nid, flag);
> +
> +	if (flag && !addr) {
> +		flag = 0;
> +		pr_warn("Could not allocate %lld bytes of mirrored memory\n", size);

dittoes.

> +		goto again;
> +	}
>  	if (!addr)
>  		return NULL;
>