From: Christoph Lameter on
On Mon, 19 Apr 2010, Minchan Kim wrote:

> Let's tidy my table.
>
> I made quick patch to show the concept with one example of pci-dma.
> (Sorry but I attach patch since web gmail's mangling.)
>
> On UMA, we can change alloc_pages with
> alloc_pages_exact_node(numa_node_id(),....)
> (Actually, the patch is already merged mmotm)

UMA does not have the concept of nodes. Whatever node you specify is
irrelevant. Please remove the patch from mmotm.

> on NUMA, alloc_pages is some different meaning, so I don't want to change it.

No it has the same meaning. It means allocate a page.

> on NUMA, alloc_pages_node means _ANY_NODE_.

It means allocate on the indicated node if possible. Memory could come
from any node due to fallback (in order of node preference).

> So let's remove nid argument and change naming with alloc_pages_any_node.

??? What in the world are you doing?

> Then, whole users of alloc_pages_node can be changed between
> alloc_pages_exact_node and alloc_pages_any_node.
>
> It was my intention. What's your concern?

I dont see the point.

> again:
> - page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
> + nid = dev_to_node(dev);
> + /*
> + * If pci-dma maintainer makes sure nid never has NUMA_NO_NODE
> + * we can remove this ugly checking.
> + */
> + if (nid == NUMA_NO_NODE)
> + page = alloc_pages_any_node(flag, get_order(size));

s/alloc_pages_any_node/alloc_pages/

> + else
> + page = alloc_pages_exact_node(nid, flag, get_order(size));

s/alloc_pages_exact_node/alloc_pages_node/

> -static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
> +static inline struct page *alloc_pagse_any_node(gfp_t gfp_mask,
> unsigned int order)
> {
> - /* Unknown node is current node */
> - if (nid < 0)
> - nid = numa_node_id();
> -
> + int nid = numa_node_id();
> return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
> }
>

This is very confusing. Because it is

alloc_pages_numa_node_id()


alloca_pages_any_node suggests that the kernel randomly picks a node?
From: Tejun Heo on
Hello, Christoph.

On 04/20/2010 02:38 AM, Christoph Lameter wrote:
> alloc_pages_exact_node results in more confusion because it does suggest
> that fallback to other nodes is not allowed.

I can't see why alloc_pages_exact_node() exists at all. It's in the
mainline and if you look at the difference between alloc_pages_node()
and alloc_pages_exact_node(), it's almost silly. :-(

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on
Hello,

On 04/20/2010 05:05 PM, Mel Gorman wrote:
> alloc_pages_exact_node() avoids a branch in a hot path that is checking for
> something the caller already knows. That's the reason it exists.

Yeah sure but Minchan is trying to tidy up the API by converting
alloc_pages_node() users to use alloc_pages_exact_node(), at which
point, the distinction becomes pretty useless. Wouldn't just making
alloc_pages_node() do what alloc_pages_exact_node() does now and
converting all its users be simpler? IIRC, the currently planned
transformation looks like the following.

alloc_pages() -> alloc_pages_any_node()
alloc_pages_node() -> basically gonna be obsoleted by _exact_node
alloc_pages_exact_node() -> gonna be used by most NUMA aware allocs

So, let's just make sure no one calls alloc_pages_node() w/ -1 nid,
kill alloc_pages_node() and rename alloc_pages_exact_node() to
alloc_pages_node().

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Minchan Kim on
On Wed, Apr 21, 2010 at 7:48 PM, Tejun Heo <tj(a)kernel.org> wrote:
> Hello,
>
> On 04/20/2010 05:05 PM, Mel Gorman wrote:
>> alloc_pages_exact_node() avoids a branch in a hot path that is checking for
>> something the caller already knows. That's the reason it exists.
>
> Yeah sure but Minchan is trying to tidy up the API by converting
> alloc_pages_node() users to use alloc_pages_exact_node(), at which
> point, the distinction becomes pretty useless.  Wouldn't just making
> alloc_pages_node() do what alloc_pages_exact_node() does now and
> converting all its users be simpler?  IIRC, the currently planned
> transformation looks like the following.
>
>  alloc_pages()                  -> alloc_pages_any_node()
>  alloc_pages_node()             -> basically gonna be obsoleted by _exact_node
>  alloc_pages_exact_node()       -> gonna be used by most NUMA aware allocs
>
> So, let's just make sure no one calls alloc_pages_node() w/ -1 nid,
> kill alloc_pages_node() and rename alloc_pages_exact_node() to
> alloc_pages_node().

Yes. It was a stupid idea. I hope Mel agree this suggestion.
Thanks for careful review, Tejun.


--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Lameter on
On Tue, 20 Apr 2010, Mel Gorman wrote:

> alloc_pages_exact_node() avoids a branch in a hot path that is checking for
> something the caller already knows. That's the reason it exists.

We can avoid alloc_pages_exact_node() by making all callers of
alloc_pages_node() never use -1. -1 is ambiguous and only rarely will a
caller pass that to alloc_pages_node().


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/