[PATCH 4/6] change alloc function in vmemmap_alloc_block
Minchan Kim
It means node's validity check is unnecessary.
So we can use alloc_pages_exact_node instead of alloc_pages_node.
It could avoid comparison and branch as 6484eb3e2a81807722 tried.
Cc: Christoph Lameter <c...@linux-foundation.org>
Signed-off-by: Minchan Kim <minch...@gmail.com>
---
mm/sparse-vmemmap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 392b9bb..7710ebc 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -53,7 +53,7 @@ void * __meminit vmemmap_alloc_block(unsigned long size, int node)
struct page *page;
if (node_state(node, N_HIGH_MEMORY))
- page = alloc_pages_node(node,
+ page = alloc_pages_exact_node(node,
GFP_KERNEL | __GFP_ZERO, get_order(size));
else
page = alloc_pages(GFP_KERNEL | __GFP_ZERO,
--
1.7.0.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Minchan Kim
It means node's validity check is unnecessary.
So we can use alloc_pages_exact_node instead of alloc_pages_node.
It could avoid comparison and branch as 6484eb3e2a81807722 tried.
Cc: Nick Piggin <npi...@suse.de>
Signed-off-by: Minchan Kim <minch...@gmail.com>
---
mm/vmalloc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index ae00746..7abf423 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1499,7 +1499,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
if (node < 0)
page = alloc_page(gfp_mask);
else
- page = alloc_pages_node(node, gfp_mask, 0);
+ page = alloc_pages_exact_node(node, gfp_mask, 0);
if (unlikely(!page)) {
/* Successfully allocated i pages, free them in __vunmap() */
Minchan Kim
It means node's validity check is unnecessary.
So we can use alloc_pages_exact_node instead of alloc_pages_node.
It could avoid comparison and branch as 6484eb3e2a81807722 tried.
Cc: Christoph Lameter <c...@linux-foundation.org>
Signed-off-by: Minchan Kim <minch...@gmail.com>
---
mm/slub.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index b364844..9984165 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1084,7 +1084,7 @@ static inline struct page *alloc_slab_page(gfp_t flags, int node,
if (node == -1)
return alloc_pages(flags, order);
else
- return alloc_pages_node(node, flags, order);
+ return alloc_pages_exact_node(node, flags, order);
}
static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
Mel Gorman
> alloc_slab_page never calls alloc_pages_node with -1.
Are you certain? What about
__kmalloc
-> slab_alloc (passed -1 as a node from __kmalloc)
-> __slab_alloc
-> new_slab
-> allocate_slab
-> alloc_slab_page
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Christoph Lameter <c...@linux-foundation.org>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
> ---
> mm/slub.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index b364844..9984165 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1084,7 +1084,7 @@ static inline struct page *alloc_slab_page(gfp_t flags, int node,
> if (node == -1)
> return alloc_pages(flags, order);
> else
> - return alloc_pages_node(node, flags, order);
> + return alloc_pages_exact_node(node, flags, order);
> }
>
> static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
> --
> 1.7.0.5
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
Minchan Kim
> On Wed, Apr 14, 2010 at 12:25:00AM +0900, Minchan Kim wrote:
>> alloc_slab_page never calls alloc_pages_node with -1.
>
> Are you certain? What about
>
> __kmalloc
> -> slab_alloc (passed -1 as a node from __kmalloc)
> -> __slab_alloc
> -> new_slab
> -> allocate_slab
> -> alloc_slab_page
>
Sorry for writing confusing changelog.
I means if node == -1 alloc_slab_page always calls alloc_pages.
So we don't need redundant check.
--
Kind regards,
Minchan Kim
Mel Gorman
> if node_state is N_HIGH_MEMORY, node doesn't have -1.
Also, if node_state is called with -1, a negative index is being checked in
a bitmap and that would be pretty broken in itself. I can't see a problem
with this patch.
Reviewed-by: Mel Gorman <m...@csn.ul.ie>
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Christoph Lameter <c...@linux-foundation.org>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
> ---
> mm/sparse-vmemmap.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index 392b9bb..7710ebc 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c
> @@ -53,7 +53,7 @@ void * __meminit vmemmap_alloc_block(unsigned long size, int node)
> struct page *page;
>
> if (node_state(node, N_HIGH_MEMORY))
> - page = alloc_pages_node(node,
> + page = alloc_pages_exact_node(node,
> GFP_KERNEL | __GFP_ZERO, get_order(size));
> else
> page = alloc_pages(GFP_KERNEL | __GFP_ZERO,
> --
> 1.7.0.5
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
Mel Gorman
> __vmalloc_area_node never pass -1 to alloc_pages_node.
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
Seems fine, the check for node id being negative has already been made.
Reviewed-by: Mel Gorman <m...@csn.ul.ie>
> Cc: Nick Piggin <npi...@suse.de>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
> ---
> mm/vmalloc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ae00746..7abf423 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1499,7 +1499,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> if (node < 0)
> page = alloc_page(gfp_mask);
> else
> - page = alloc_pages_node(node, gfp_mask, 0);
> + page = alloc_pages_exact_node(node, gfp_mask, 0);
>
> if (unlikely(!page)) {
> /* Successfully allocated i pages, free them in __vunmap() */
> --
> 1.7.0.5
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
Mel Gorman
> On Wed, Apr 14, 2010 at 12:52 AM, Mel Gorman <m...@csn.ul.ie> wrote:
> > On Wed, Apr 14, 2010 at 12:25:00AM +0900, Minchan Kim wrote:
> >> alloc_slab_page never calls alloc_pages_node with -1.
> >
> > Are you certain? What about
> >
> > __kmalloc
> > ￜ ￜ-> __slab_alloc
> > ￜ ￜ ￜ-> new_slab
> > ￜ ￜ ￜ ￜ-> allocate_slab
> > ￜ ￜ ￜ ￜ ￜ-> alloc_slab_page
> >
>
> Sorry for writing confusing changelog.
>
> I means if node == -1 alloc_slab_page always calls alloc_pages.
> So we don't need redundant check.
>
When the changelog is fixed up, feel free to add;
Reviewed-by: Mel Gorman <m...@csn.ul.ie>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
David Rientjes
> alloc_slab_page never calls alloc_pages_node with -1.
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Christoph Lameter <c...@linux-foundation.org>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
> ---
> mm/slub.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index b364844..9984165 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1084,7 +1084,7 @@ static inline struct page *alloc_slab_page(gfp_t flags, int node,
> if (node == -1)
> return alloc_pages(flags, order);
> else
> - return alloc_pages_node(node, flags, order);
> + return alloc_pages_exact_node(node, flags, order);
> }
>
> static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
Slub changes need to go through its maintainer, Pekka Enberg
<pen...@cs.helsinki.fi>.
Minchan Kim
> On Wed, 14 Apr 2010, Minchan Kim wrote:
>
>> alloc_slab_page never calls alloc_pages_node with -1.
>> It means node's validity check is unnecessary.
>> So we can use alloc_pages_exact_node instead of alloc_pages_node.
>> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>>
>> Cc: Christoph Lameter <c...@linux-foundation.org>
>> Signed-off-by: Minchan Kim <minch...@gmail.com>
>> ---
>> mm/slub.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index b364844..9984165 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1084,7 +1084,7 @@ static inline struct page *alloc_slab_page(gfp_t flags, int node,
>> if (node == -1)
>> return alloc_pages(flags, order);
>> else
>> - return alloc_pages_node(node, flags, order);
>> + return alloc_pages_exact_node(node, flags, order);
>> }
>>
>> static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>
> Slub changes need to go through its maintainer, Pekka Enberg
> <pen...@cs.helsinki.fi>.
Thanks, David. It was by mistake.
Pekka.
This changlog is bad.
I will change it with following as when I send v2.
"alloc_slab_page always checks nid == -1, so alloc_page_node can't be
called with -1.
It means node's validity check in alloc_pages_node is unnecessary.
So we can use alloc_pages_exact_node instead of alloc_pages_node.
It could avoid comparison and branch as 6484eb3e2a81807722 tried."
Thanks.
--
Kind regards,
Minchan Kim
David Rientjes
> This changlog is bad.
> I will change it with following as when I send v2.
>
> "alloc_slab_page always checks nid == -1, so alloc_page_node can't be
> called with -1.
> It means node's validity check in alloc_pages_node is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried."
>
Each patch in this series seems to be independent and can be applied
seperately, so it's probably not necessary to make them incremental.
Minchan Kim
> On Wed, 14 Apr 2010, Minchan Kim wrote:
>
>> This changlog is bad.
>> I will change it with following as when I send v2.
>>
>> "alloc_slab_page always checks nid == -1, so alloc_page_node can't be
>> called with -1.
>> It means node's validity check in alloc_pages_node is unnecessary.
>> So we can use alloc_pages_exact_node instead of alloc_pages_node.
>> It could avoid comparison and branch as 6484eb3e2a81807722 tried."
>>
>
> Each patch in this series seems to be independent and can be applied
> seperately, so it's probably not necessary to make them incremental.
Surely.
Without considering, I used git-formant-patch -n --thread.
I will keep it in mind.
Thanks, David.
--
Kind regards,
Minchan Kim
KAMEZAWA Hiroyuki
Minchan Kim <minch...@gmail.com> wrote:
> alloc_slab_page never calls alloc_pages_node with -1.
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Christoph Lameter <c...@linux-foundation.org>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
Reviewed-by: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
KAMEZAWA Hiroyuki
Minchan Kim <minch...@gmail.com> wrote:
> __vmalloc_area_node never pass -1 to alloc_pages_node.
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Nick Piggin <npi...@suse.de>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
Reviewed-by: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
But, in another thinking,
- if (node < 0)
- page = alloc_page(gfp_mask);
may be better ;)
Thanks,
-Kame
KAMEZAWA Hiroyuki
Minchan Kim <minch...@gmail.com> wrote:
> if node_state is N_HIGH_MEMORY, node doesn't have -1.
> It means node's validity check is unnecessary.
> So we can use alloc_pages_exact_node instead of alloc_pages_node.
> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>
> Cc: Christoph Lameter <c...@linux-foundation.org>
> Signed-off-by: Minchan Kim <minch...@gmail.com>
Reviewed-by: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Minchan Kim
<kamezaw...@jp.fujitsu.com> wrote:
> On Wed, 14 Apr 2010 00:25:02 +0900
> Minchan Kim <minch...@gmail.com> wrote:
>
>> __vmalloc_area_node never pass -1 to alloc_pages_node.
>> It means node's validity check is unnecessary.
>> So we can use alloc_pages_exact_node instead of alloc_pages_node.
>> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>>
>> Cc: Nick Piggin <npi...@suse.de>
>> Signed-off-by: Minchan Kim <minch...@gmail.com>
>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
>
> But, in another thinking,
>
> - if (node < 0)
> - page = alloc_page(gfp_mask);
>
> may be better ;)
I thought it.
but alloc_page is different function with alloc_pages_node in NUMA.
It calls alloc_pages_current.
--
Kind regards,
Minchan Kim
Pekka Enberg
<kamezaw...@jp.fujitsu.com> wrote:
> On Wed, 14 Apr 2010 00:25:00 +0900
> Minchan Kim <minch...@gmail.com> wrote:
>
>> alloc_slab_page never calls alloc_pages_node with -1.
>> It means node's validity check is unnecessary.
>> So we can use alloc_pages_exact_node instead of alloc_pages_node.
>> It could avoid comparison and branch as 6484eb3e2a81807722 tried.
>>
>> Cc: Christoph Lameter <c...@linux-foundation.org>
>> Signed-off-by: Minchan Kim <minch...@gmail.com>
>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Minchan, care to send a v2 with proper changelog and reviewed-by attributions?
Christoph Lameter
> Minchan, care to send a v2 with proper changelog and reviewed-by attributions?
Still wondering what the big deal about alloc_pages_node_exact is. Its not
exact since we can fall back to another node. It is better to clarify the
API for alloc_pages_node and forbid / clarify the use of -1.
Pekka Enberg
> On Wed, 14 Apr 2010, Pekka Enberg wrote:
>
>> Minchan, care to send a v2 with proper changelog and reviewed-by attributions?
>
> Still wondering what the big deal about alloc_pages_node_exact is. Its not
> exact since we can fall back to another node. It is better to clarify the
> API for alloc_pages_node and forbid / clarify the use of -1.
Minchan's patch is a continuation of this patch:
http://git.kernel.org/?p=linux/kernel/git/penberg/slab-2.6.git;a=commit;h=6484eb3e2a81807722
Mel Gorman
> On Wed, 14 Apr 2010, Pekka Enberg wrote:
>
> > Minchan, care to send a v2 with proper changelog and reviewed-by attributions?
>
> Still wondering what the big deal about alloc_pages_node_exact is. Its not
> exact since we can fall back to another node. It is better to clarify the
> API for alloc_pages_node and forbid / clarify the use of -1.
>
There should be a comment clarifing it now. I admit the naming fault is
mine. At the time, the intended meaning was "allocate pages from any node in
the fallback list and the caller knows exactly which node to start from". I
did not take into account that the meaning of "exact" depends on context.
With a comment clarifying the meaning, I do not think a rename is necessary.
However, I'd rather not see a mass renaming of functions like alloc_pages()
that have existed a long times. If nothing else, they are documented in books
like "Linux Device Drivers" so why make life harder on device authors than
it already is?
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab