lkml.org 
[lkml]   [2013]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 06/23] mm/memblock: Add memblock early memory allocation apis
Hello,

On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
> Introduce memblock early memory allocation APIs which allow to support
> LPAE extension on 32 bits archs. More over, this is the next step

LPAE isn't something people outside arm circle would understand.
Let's stick to highmem.

> to get rid of NO_BOOTMEM memblock wrapper(nobootmem.c) and directly use
> memblock APIs.
>
> The proposed interface will became active if both CONFIG_HAVE_MEMBLOCK
> and CONFIG_NO_BOOTMEM are specified by arch. In case !CONFIG_NO_BOOTMEM,
> the memblock() wrappers will fallback to the existing bootmem apis so
> that arch's noy converted to NO_BOOTMEM continue to work as is.
^^^
typo

> +/* FIXME: Move to memblock.h at a point where we remove nobootmem.c */
> +void *memblock_early_alloc_try_nid_nopanic(int nid, phys_addr_t size,
> + phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);
> +void *memblock_early_alloc_try_nid(int nid, phys_addr_t size,
> + phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);

Wouldn't it make more sense to put @nid at the end. @size is the main
parameter here and it gets confusing with _alloc_node() interface as
the positions of paramters change. Plus, kmalloc_node() puts @node at
the end too.

> +void __memblock_free_early(phys_addr_t base, phys_addr_t size);
> +void __memblock_free_late(phys_addr_t base, phys_addr_t size);

Would it be possible to drop "early"? It's redundant and makes the
function names unnecessarily long. When memblock is enabled, these
are basically doing about the same thing as memblock_alloc() and
friends, right? Wouldn't it make more sense to define these as
memblock_alloc_XXX()?

> +#define memblock_early_alloc(x) \
> + memblock_early_alloc_try_nid(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_align(x, align) \
> + memblock_early_alloc_try_nid(MAX_NUMNODES, x, align, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_nopanic(x) \
> + memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_pages(x) \
> + memblock_early_alloc_try_nid(MAX_NUMNODES, x, PAGE_SIZE, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_pages_nopanic(x) \
> + memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, PAGE_SIZE, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)

I always felt a bit weird about _pages() interface. It says pages but
takes bytes in size. Maybe we're better off just converting the
current _pages users to _alloc_align()?

> +#define memblock_early_alloc_node(nid, x) \
> + memblock_early_alloc_try_nid(nid, x, SMP_CACHE_BYTES, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_node_nopanic(nid, x) \
> + memblock_early_alloc_try_nid_nopanic(nid, x, SMP_CACHE_BYTES, \
> + BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)

Ditto as above. Maybe @nid can be moved to the end?

> +static void * __init _memblock_early_alloc_try_nid_nopanic(int nid,
> + phys_addr_t size, phys_addr_t align,
> + phys_addr_t from, phys_addr_t max_addr)
> +{
> + phys_addr_t alloc;
> + void *ptr;
> +
> + if (WARN_ON_ONCE(slab_is_available())) {
> + if (nid == MAX_NUMNODES)

Shouldn't we be using NUMA_NO_NODE?

> + return kzalloc(size, GFP_NOWAIT);
> + else
> + return kzalloc_node(size, GFP_NOWAIT, nid);

And kzalloc_node() understands NUMA_NO_NODE.

> + }
> +
> + if (WARN_ON(!align))
> + align = __alignof__(long long);

Wouldn't SMP_CACHE_BYTES make more sense? Also, I'm not sure we
actually want WARN on it. Interpreting 0 as "default align" isn't
that weird.

> + /* align @size to avoid excessive fragmentation on reserved array */
> + size = round_up(size, align);
> +
> +again:
> + alloc = memblock_find_in_range_node(from, max_addr, size, align, nid);
> + if (alloc)
> + goto done;
> +
> + if (nid != MAX_NUMNODES) {
> + alloc =
> + memblock_find_in_range_node(from, max_addr, size,
> + align, MAX_NUMNODES);
> + if (alloc)
> + goto done;
> + }
> +
> + if (from) {
> + from = 0;
> + goto again;
> + } else {
> + goto error;
> + }
> +
> +done:
> + memblock_reserve(alloc, size);
> + ptr = phys_to_virt(alloc);
> + memset(ptr, 0, size);

What if the address is high? Don't we need kmapping here?

> +
> + /*
> + * The min_count is set to 0 so that bootmem allocated blocks
> + * are never reported as leaks.
> + */
> + kmemleak_alloc(ptr, size, 0, 0);
> +
> + return ptr;
> +
> +error:
> + return NULL;
> +}
> +
> +void * __init memblock_early_alloc_try_nid_nopanic(int nid,
> + phys_addr_t size, phys_addr_t align,
> + phys_addr_t from, phys_addr_t max_addr)
> +{
> + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
> + __func__, (u64)size, (u64)align, nid, (u64)from,
> + (u64)max_addr, (void *)_RET_IP_);
> + return _memblock_early_alloc_try_nid_nopanic(nid, size,
> + align, from, max_addr);

Do we need the extra level of wrapping? Just implement
alloc_try_nid_nopanic() here and make the panicky version call it?

Thanks.

--
tejun


\
 
 \ /
  Last update: 2013-10-13 20:21    [W:0.194 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site