2010-08-04 02:52:18

by Christoph Lameter

[permalink] [raw]
Subject: [S+Q3 03/23] slub: Use a constant for a unspecified node.

kmalloc_node() and friends can be passed a constant -1 to indicate
that no choice was made for the node from which the object needs to
come.

Use NUMA_NO_NODE instead of -1.

CC: KAMEZAWA Hiroyuki <[email protected]>
Signed-off-by: David Rientjes <[email protected]>
Signed-off-by: Christoph Lameter <[email protected]>

---
mm/slub.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c 2010-07-26 12:57:52.000000000 -0500
+++ linux-2.6/mm/slub.c 2010-07-26 12:57:59.000000000 -0500
@@ -1073,7 +1073,7 @@ static inline struct page *alloc_slab_pa

flags |= __GFP_NOTRACK;

- if (node == -1)
+ if (node == NUMA_NO_NODE)
return alloc_pages(flags, order);
else
return alloc_pages_exact_node(node, flags, order);
@@ -1387,7 +1387,7 @@ static struct page *get_any_partial(stru
static struct page *get_partial(struct kmem_cache *s, gfp_t flags, int node)
{
struct page *page;
- int searchnode = (node == -1) ? numa_node_id() : node;
+ int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;

page = get_partial_node(get_node(s, searchnode));
if (page || (flags & __GFP_THISNODE) || node != -1)
@@ -1515,7 +1515,7 @@ static void flush_all(struct kmem_cache
static inline int node_match(struct kmem_cache_cpu *c, int node)
{
#ifdef CONFIG_NUMA
- if (node != -1 && c->node != node)
+ if (node != NUMA_NO_NODE && c->node != node)
return 0;
#endif
return 1;
@@ -1727,7 +1727,7 @@ static __always_inline void *slab_alloc(

void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
{
- void *ret = slab_alloc(s, gfpflags, -1, _RET_IP_);
+ void *ret = slab_alloc(s, gfpflags, NUMA_NO_NODE, _RET_IP_);

trace_kmem_cache_alloc(_RET_IP_, ret, s->objsize, s->size, gfpflags);

@@ -1738,7 +1738,7 @@ EXPORT_SYMBOL(kmem_cache_alloc);
#ifdef CONFIG_TRACING
void *kmem_cache_alloc_notrace(struct kmem_cache *s, gfp_t gfpflags)
{
- return slab_alloc(s, gfpflags, -1, _RET_IP_);
+ return slab_alloc(s, gfpflags, NUMA_NO_NODE, _RET_IP_);
}
EXPORT_SYMBOL(kmem_cache_alloc_notrace);
#endif
@@ -2728,7 +2728,7 @@ void *__kmalloc(size_t size, gfp_t flags
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;

- ret = slab_alloc(s, flags, -1, _RET_IP_);
+ ret = slab_alloc(s, flags, NUMA_NO_NODE, _RET_IP_);

trace_kmalloc(_RET_IP_, ret, size, s->size, flags);

@@ -3312,7 +3312,7 @@ void *__kmalloc_track_caller(size_t size
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;

- ret = slab_alloc(s, gfpflags, -1, caller);
+ ret = slab_alloc(s, gfpflags, NUMA_NO_NODE, caller);

/* Honor the call site pointer we recieved. */
trace_kmalloc(caller, ret, size, s->size, gfpflags);


2010-08-04 03:34:14

by David Rientjes

[permalink] [raw]
Subject: Re: [S+Q3 03/23] slub: Use a constant for a unspecified node.

On Tue, 3 Aug 2010, Christoph Lameter wrote:

> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c 2010-07-26 12:57:52.000000000 -0500
> +++ linux-2.6/mm/slub.c 2010-07-26 12:57:59.000000000 -0500
> @@ -1073,7 +1073,7 @@ static inline struct page *alloc_slab_pa
>
> flags |= __GFP_NOTRACK;
>
> - if (node == -1)
> + if (node == NUMA_NO_NODE)
> return alloc_pages(flags, order);
> else
> return alloc_pages_exact_node(node, flags, order);
> @@ -1387,7 +1387,7 @@ static struct page *get_any_partial(stru
> static struct page *get_partial(struct kmem_cache *s, gfp_t flags, int node)
> {
> struct page *page;
> - int searchnode = (node == -1) ? numa_node_id() : node;
> + int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;
>
> page = get_partial_node(get_node(s, searchnode));
> if (page || (flags & __GFP_THISNODE) || node != -1)

This has a merge conflict with 2.6.35 since it has this:

page = get_partial_node(get_node(s, searchnode));
if (page || (flags & __GFP_THISNODE))
return page;

return get_any_partial(s, flags);

so what happened to the dropped check for returning get_any_partial() when
node != -1? I added the check for benchmarking.

2010-08-04 16:15:35

by Christoph Lameter

[permalink] [raw]
Subject: Re: [S+Q3 03/23] slub: Use a constant for a unspecified node.

On Tue, 3 Aug 2010, David Rientjes wrote:

> > static struct page *get_partial(struct kmem_cache *s, gfp_t flags, int node)
> > {
> > struct page *page;
> > - int searchnode = (node == -1) ? numa_node_id() : node;
> > + int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;
> >
> > page = get_partial_node(get_node(s, searchnode));
> > if (page || (flags & __GFP_THISNODE) || node != -1)
>
> This has a merge conflict with 2.6.35 since it has this:
>
> page = get_partial_node(get_node(s, searchnode));
> if (page || (flags & __GFP_THISNODE))
> return page;
>
> return get_any_partial(s, flags);
>
> so what happened to the dropped check for returning get_any_partial() when
> node != -1? I added the check for benchmarking.

Strange no merge conflict here. Are you sure you use upstream?

GFP_THISNODE does not matter too much. If page == NULL then we failed
to allocate a page on a specific node and have to either give up (and then
extend the slab) or take a page from another node.

We always have give up to go to the page allocator if GFP_THIS_NODE was
set. The modification to additionally also go to the page allocator if
a node was just set even without GFP_THISNODE. So checking for
GFP_THISNODE does not make sense anymore.



2010-08-05 07:40:45

by David Rientjes

[permalink] [raw]
Subject: Re: [S+Q3 03/23] slub: Use a constant for a unspecified node.

On Wed, 4 Aug 2010, Christoph Lameter wrote:

> > > static struct page *get_partial(struct kmem_cache *s, gfp_t flags, int node)
> > > {
> > > struct page *page;
> > > - int searchnode = (node == -1) ? numa_node_id() : node;
> > > + int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;
> > >
> > > page = get_partial_node(get_node(s, searchnode));
> > > if (page || (flags & __GFP_THISNODE) || node != -1)
> >
> > This has a merge conflict with 2.6.35 since it has this:
> >
> > page = get_partial_node(get_node(s, searchnode));
> > if (page || (flags & __GFP_THISNODE))
> > return page;
> >
> > return get_any_partial(s, flags);
> >
> > so what happened to the dropped check for returning get_any_partial() when
> > node != -1? I added the check for benchmarking.
>
> Strange no merge conflict here. Are you sure you use upstream?
>

Yes, 2.6.35 does not have the node != -1 check and Linus hasn't pulled
slub/fixes from Pekka's tree yet. Even when he does, "slub numa: Fix rare
allocation from unexpected node" removes the __GFP_THISNODE check before
adding node != -1, so this definitely doesn't apply to anybody else's
tree.