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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>