[RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Eric Dumazet]

In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path length, we could exploit the following common facts.

1) MAX_NUMNODES <= 64

2) alignment of 'struct kmem_cache *' can be >= 64

The following patch changes the page->lru.next to contain not only the 'struct kmem_cache *' pointer, but also the nodeid in the low order bits.

This also reduces sizeof(struct slab) by 8 bytes on 64bits arches.
This reduces sizeof(struct slab) on all platforms (UP, or SMP)

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

diff --git a/mm/slab.c b/mm/slab.c
index abf46ae..d2f7299 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -210,6 +210,16 @@ #define BUFCTL_FREE	(((kmem_bufctl_t)(~0
 #define	BUFCTL_ACTIVE	(((kmem_bufctl_t)(~0U))-2)
 #define	SLAB_LIMIT	(((kmem_bufctl_t)(~0U))-3)
 
+#ifdef CONFIG_NUMA
+#if MAX_NUMNODES <= 64
+/* we can use low order bits of page->lru.next to store nodeids */
+# define KEEP_NODEID_IN_PAGE
+#else
+/* too many nodes, we need a field in 'struct slab' */
+# define KEEP_NODEID_IN_SLAB
+#endif
+#endif
+
 /*
  * struct slab
  *
@@ -223,7 +233,9 @@ struct slab {
 	void *s_mem;		/* including colour offset */
 	unsigned int inuse;	/* num of objs active in slab */
 	kmem_bufctl_t free;
+#ifdef KEEP_NODEID_IN_SLAB
 	unsigned short nodeid;
+#endif
 };
 
 /*
@@ -585,9 +597,18 @@ static int slab_break_gfp_order = BREAK_
  * allocator.  These are used to find the slab an obj belongs to.  With kfree(),
  * these are used to find the cache which an obj belongs to.
  */
-static inline void page_set_cache(struct page *page, struct kmem_cache *cache)
+static inline void page_set_cache_slab_nodeid(struct page *page,
+	struct kmem_cache *cache, struct slab *slab, int nodeid)
 {
+	page->lru.prev = (struct list_head *)slab;
+#ifdef KEEP_NODEID_IN_PAGE
+	page->lru.next = (struct list_head *)((long)cache + nodeid);
+#else
 	page->lru.next = (struct list_head *)cache;
+#endif
+#ifdef KEEP_NODEID_IN_SLAB
+	slab->nodeid = nodeid;
+#endif
 }
 
 static inline struct kmem_cache *page_get_cache(struct page *page)
@@ -595,12 +616,11 @@ static inline struct kmem_cache *page_ge
 	if (unlikely(PageCompound(page)))
 		page = (struct page *)page_private(page);
 	BUG_ON(!PageSlab(page));
+#ifdef KEEP_NODEID_IN_PAGE
+	return (struct kmem_cache *)((long)page->lru.next & ~63);
+#else
 	return (struct kmem_cache *)page->lru.next;
-}
-
-static inline void page_set_slab(struct page *page, struct slab *slab)
-{
-	page->lru.prev = (struct list_head *)slab;
+#endif
 }
 
 static inline struct slab *page_get_slab(struct page *page)
@@ -617,6 +637,18 @@ static inline struct kmem_cache *virt_to
 	return page_get_cache(page);
 }
 
+#ifdef CONFIG_NUMA
+static inline int virt_to_nodeid(const void *obj)
+{
+	struct page *page = virt_to_page(obj);
+#ifdef KEEP_NODEID_IN_SLAB
+	return page_get_slab(page)->nodeid;
+#else
+	return (long)page->lru.next & 63;
+#endif
+}
+#endif
+
 static inline struct slab *virt_to_slab(const void *obj)
 {
 	struct page *page = virt_to_page(obj);
@@ -1134,8 +1166,7 @@ static void drain_alien_cache(struct kme
 
 static inline int cache_free_alien(struct kmem_cache *cachep, void *objp)
 {
-	struct slab *slabp = virt_to_slab(objp);
-	int nodeid = slabp->nodeid;
+	int nodeid = virt_to_nodeid(objp);
 	struct kmem_list3 *l3;
 	struct array_cache *alien = NULL;
 	int node;
@@ -1146,7 +1177,7 @@ static inline int cache_free_alien(struc
 	 * Make sure we are not freeing a object from another node to the array
 	 * cache on this cpu.
 	 */
-	if (likely(slabp->nodeid == node) || unlikely(!use_alien_caches))
+	if (likely(nodeid == node) || unlikely(!use_alien_caches))
 		return 0;
 
 	l3 = cachep->nodelists[node];
@@ -1437,8 +1468,14 @@ void __init kmem_cache_init(void)
 	cache_cache.array[smp_processor_id()] = &initarray_cache.cache;
 	cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];
 
+#ifdef KEEP_NODEID_IN_PAGE
+	/* kmem_cache addresses must be multiple of 64 */
+	cache_cache.buffer_size = ALIGN(cache_cache.buffer_size,
+					max(64, cache_line_size()));
+#else
 	cache_cache.buffer_size = ALIGN(cache_cache.buffer_size,
 					cache_line_size());
+#endif
 	cache_cache.reciprocal_buffer_size =
 		reciprocal_value(cache_cache.buffer_size);
 
@@ -2588,7 +2625,6 @@ static struct slab *alloc_slabmgmt(struc
 	slabp->inuse = 0;
 	slabp->colouroff = colour_off;
 	slabp->s_mem = objp + colour_off;
-	slabp->nodeid = nodeid;
 	return slabp;
 }
 
@@ -2699,7 +2735,7 @@ #endif
  * virtual address for kfree, ksize, kmem_ptr_validate, and slab debugging.
  */
 static void slab_map_pages(struct kmem_cache *cache, struct slab *slab,
-			   void *addr)
+			   void *addr, int nodeid)
 {
 	int nr_pages;
 	struct page *page;
@@ -2711,8 +2747,7 @@ static void slab_map_pages(struct kmem_c
 		nr_pages <<= cache->gfporder;
 
 	do {
-		page_set_cache(page, cache);
-		page_set_slab(page, slab);
+		page_set_cache_slab_nodeid(page, cache, slab, nodeid);
 		page++;
 	} while (--nr_pages);
 }
@@ -2787,8 +2822,7 @@ static int cache_grow(struct kmem_cache 
 	if (!slabp)
 		goto opps1;
 
-	slabp->nodeid = nodeid;
-	slab_map_pages(cachep, slabp, objp);
+	slab_map_pages(cachep, slabp, objp, nodeid);
 
 	cache_init_objs(cachep, slabp, ctor_flags);
 

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka Enberg]

On 3/21/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path
> length, we could exploit the following common facts.

It would be nice if you could cc me for slab patches.

On 3/21/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> -static inline void page_set_cache(struct page *page, struct kmem_cache *cache)
> +static inline void page_set_cache_slab_nodeid(struct page *page,
> +       struct kmem_cache *cache, struct slab *slab, int nodeid)
>  {
> +       page->lru.prev = (struct list_head *)slab;
> +#ifdef KEEP_NODEID_IN_PAGE
> +       page->lru.next = (struct list_head *)((long)cache + nodeid);
> +#else
>         page->lru.next = (struct list_head *)cache;
> +#endif
> +#ifdef KEEP_NODEID_IN_SLAB
> +       slab->nodeid = nodeid;
> +#endif

Can we please have a slab_get_nid() and slab_set_nid() instead which
reduces the need for #ifdefs?

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka Enberg]

On 3/21/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> +#ifdef KEEP_NODEID_IN_PAGE
> +       /* kmem_cache addresses must be multiple of 64 */
> +       cache_cache.buffer_size = ALIGN(cache_cache.buffer_size,
> +                                       max(64, cache_line_size()));
> +#else
>         cache_cache.buffer_size = ALIGN(cache_cache.buffer_size,
>                                         cache_line_size());
> +#endif

Please introduce cache_cache_align() to hide the #ifdeffing.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Christoph Lameter]

On Wed, 21 Mar 2007, Eric Dumazet wrote:

> In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path length, we could exploit the following common facts.
> 
> 1) MAX_NUMNODES <= 64

MAX_NUMNODES == 1024 on IA64 and may be on x86_64 in the future.
 
> 2) alignment of 'struct kmem_cache *' can be >= 64
> 
> The following patch changes the page->lru.next to contain not only the 
> 'struct kmem_cache *' pointer, but also the nodeid in the low order 
> bits.

Nack.

I would suggest you audit slab and make sure that we never fall back and 
always enter a slab on the node of page_to_nid(page). If 
all allocations use GFP_THISNODE then you should be safe and then we can 
remove the nodeid from the slab structure and simply use the available 
node information in the page struct.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Christoph Lameter]

On Wed, 21 Mar 2007, Pekka Enberg wrote:

> Can we please have a slab_get_nid() and slab_set_nid() instead which
> reduces the need for #ifdefs?

None of that please. The page flags already contain a node number that is 
accessible via page_to_nid(). Just make sure that we never put a slab onto 
the wrong node.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka Enberg]

On 3/21/07, Christoph Lameter <christoph@lameter.com> wrote:
> None of that please. The page flags already contain a node number that is
> accessible via page_to_nid(). Just make sure that we never put a slab onto
> the wrong node.

Sounds good. Do you know for a fact that we don't or are you just
afraid we have a bug?

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Eric Dumazet]

On Wed, 21 Mar 2007 07:41:46 -0700 (PDT)
Christoph Lameter <christoph@lameter.com> wrote:

> On Wed, 21 Mar 2007, Eric Dumazet wrote:
> 
> > In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path length, we could exploit the following common facts.
> > 
> > 1) MAX_NUMNODES <= 64
> 
> MAX_NUMNODES == 1024 on IA64 and may be on x86_64 in the future.

So what ? I am very happy for you Christoph.

Average linux machines (ie more than 99.99 % on this planet) are using MAX_NUMNODES <= 64

If you read my patch, your lovely 1024 nodes machines will run as before : No change at all for big machines.

>  
> > 2) alignment of 'struct kmem_cache *' can be >= 64
> > 
> > The following patch changes the page->lru.next to contain not only the 
> > 'struct kmem_cache *' pointer, but also the nodeid in the low order 
> > bits.
> 
> Nack.
> 
> I would suggest you audit slab and make sure that we never fall back and 
> always enter a slab on the node of page_to_nid(page). If 
> all allocations use GFP_THISNODE then you should be safe and then we can 
> remove the nodeid from the slab structure and simply use the available 
> node information in the page struct.
> 

Last time I checked 'struct page', they was no nodeid in it.

I cooked this patch because you told me it was possible to have a 'slab nodeid' different than page_to_nid(obj). Now you want me to prove the contrary. Me confused.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka J Enberg]

On Wed, 21 Mar 2007, Christoph Lameter wrote:
> None of that please. The page flags already contain a node number that is 
> accessible via page_to_nid(). Just make sure that we never put a slab onto 
> the wrong node.

Oh well, here's a patch to find out. (I don't have a NUMA box.)

			Pekka

From: Pekka Enberg <penberg@cs.helsinki.fi>

This adds a virt_to_nodeid() for looking up the NUMA node ID for a
slab object. The current implementation uses slab->nodeid but adds a
WARN_ON to catch cases where page nid differs. Eventually, we can
change virt_to_nodeid() to use page_to_nid after which slab->nodeid
can be removed.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slab.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c	2007-03-21 16:58:53.000000000 +0200
+++ 2.6/mm/slab.c	2007-03-21 16:59:26.000000000 +0200
@@ -623,6 +623,14 @@ static inline struct slab *virt_to_slab(
 	return page_get_slab(page);
 }
 
+static inline unsigned short virt_to_nodeid(const void *obj)
+{
+	struct page *page = virt_to_page(obj);
+	struct slab *slab = page_get_slab(page);
+	WARN_ON(slab->nodeid != page_to_nid(page));
+	return slab->nodeid;
+}
+
 static inline void *index_to_obj(struct kmem_cache *cache, struct slab *slab,
 				 unsigned int idx)
 {
@@ -1134,8 +1142,7 @@ 	int i = 0;
 
 static inline int cache_free_alien(struct kmem_cache *cachep, void *objp)
 {
-	struct slab *slabp = virt_to_slab(objp);
-	int nodeid = slabp->nodeid;
+	int nodeid = virt_to_nodeid(objp);
 	struct kmem_list3 *l3;
 	struct array_cache *alien = NULL;
 	int node;
@@ -1146,7 +1153,7 @@ static inline int cache_free_alien(struc
 	 * Make sure we are not freeing a object from another node to the array
 	 * cache on this cpu.
 	 */
-	if (likely(slabp->nodeid == node) || unlikely(!use_alien_caches))
+	if (likely(nodeid == node) || unlikely(!use_alien_caches))
 		return 0;
 
 	l3 = cachep->nodelists[node];
@@ -2665,7 +2672,7 @@ static void *slab_get_obj(struct kmem_ca
 	next = slab_bufctl(slabp)[slabp->free];
 #if DEBUG
 	slab_bufctl(slabp)[slabp->free] = BUFCTL_FREE;
-	WARN_ON(slabp->nodeid != nodeid);
+	WARN_ON(virt_to_nodeid(objp) != nodeid);
 #endif
 	slabp->free = next;
 
@@ -2679,7 +2686,7 @@ static void slab_put_obj(struct kmem_cac
 
 #if DEBUG
 	/* Verify that the slab belongs to the intended node */
-	WARN_ON(slabp->nodeid != nodeid);
+	WARN_ON(virt_to_nodeid(objp) != nodeid);
 
 	if (slab_bufctl(slabp)[objnr] + 1 <= SLAB_LIMIT + 1) {
 		printk(KERN_ERR "slab: double free detected in cache "

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Christoph Lameter]

On Wed, 21 Mar 2007, Pekka Enberg wrote:

> On 3/21/07, Christoph Lameter <christoph@lameter.com> wrote:
> > None of that please. The page flags already contain a node number that is
> > accessible via page_to_nid(). Just make sure that we never put a slab onto
> > the wrong node.
> 
> Sounds good. Do you know for a fact that we don't or are you just
> afraid we have a bug?

I tried to clean it up during the GFP_THISNODE work but there was some 
issue on powerpc where we needed fallback for bootstrap. There may be more 
trouble there. Checking the allocs where we do not use GFP_THISNODE should 
get you there.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Christoph Lameter]

On Wed, 21 Mar 2007, Eric Dumazet wrote:

> On Wed, 21 Mar 2007 07:41:46 -0700 (PDT)
> Christoph Lameter <christoph@lameter.com> wrote:
> 
> > On Wed, 21 Mar 2007, Eric Dumazet wrote:
> > 
> > > In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path length, we could exploit the following common facts.
> > > 
> > > 1) MAX_NUMNODES <= 64
> > 
> > MAX_NUMNODES == 1024 on IA64 and may be on x86_64 in the future.
> 
> So what ? I am very happy for you Christoph.

You wanted to exploit that MAX_NUMNODES <= 64?

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka Enberg]

On 3/21/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Last time I checked 'struct page', they was no nodeid in it.

Hmm, page_to_nid() in include/linx/mm.h doesn't agree with you:

#ifdef NODE_NOT_IN_PAGE_FLAGS
extern int page_to_nid(struct page *page);
#else
static inline int page_to_nid(struct page *page)
{
        return (page->flags >> NODES_PGSHIFT) & NODES_MASK;
}
#endif

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Eric Dumazet]

On Wed, 21 Mar 2007 17:42:22 +0200 (EET)
Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> On Wed, 21 Mar 2007, Christoph Lameter wrote:
> > None of that please. The page flags already contain a node number that is 
> > accessible via page_to_nid(). Just make sure that we never put a slab onto 
> > the wrong node.
> 
> Oh well, here's a patch to find out. (I don't have a NUMA box.)
> 

This wont work, because of use_alien_caches knob.

If use_alien_caches is set to 0, your WARN_ON will trigger many false alarms

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Eric Dumazet]

On Wed, 21 Mar 2007 17:47:13 +0200
"Pekka Enberg" <penberg@cs.helsinki.fi> wrote:

> On 3/21/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> > Last time I checked 'struct page', they was no nodeid in it.
> 
> Hmm, page_to_nid() in include/linx/mm.h doesn't agree with you:
> 
> #ifdef NODE_NOT_IN_PAGE_FLAGS
> extern int page_to_nid(struct page *page);
> #else
> static inline int page_to_nid(struct page *page)
> {
>         return (page->flags >> NODES_PGSHIFT) & NODES_MASK;
> }
> #endif
> 

Correct, but still another ifdef NODE_NOT_IN_PAGE_FLAGS

Anyway the correct path is to check GFP_THISNODE stuff, because we can
even avoid access to 'struct page' to get the nodeid, at least on x86_64 NUMA,
where struct memnode can provide the information with less cache footprint.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka J Enberg]

On Wed, 21 Mar 2007, Eric Dumazet wrote:
> This wont work, because of use_alien_caches knob.
> 
> If use_alien_caches is set to 0, your WARN_ON will trigger many false 
> alarms

How is that? The mapping should always be the same regardless of whether 
we use alien caches or not.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Eric Dumazet]

On Wed, 21 Mar 2007 08:44:53 -0700 (PDT)
Christoph Lameter <christoph@lameter.com> wrote:
> You wanted to exploit that MAX_NUMNODES <= 64?

Maybe my english is not perfect. I promise I improve it eventually.

My patch was for unlucky guys that have MAX_NUMNODES <= 64.

For others, the preprocessor gave the old code.

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Andi Kleen]

On Wed, Mar 21, 2007 at 10:22:31AM +0100, Eric Dumazet wrote:
> In order to avoid a cache miss in kmem_cache_free() on NUMA and reduce hot path length, we could exploit the following common facts.
> 
> 1) MAX_NUMNODES <= 64
> 
> 2) alignment of 'struct kmem_cache *' can be >= 64
> 
> The following patch changes the page->lru.next to contain not only the 'struct kmem_cache *' pointer, but also the nodeid in the low order bits.
> 

It might be cleaner to just put it into flags on 64bit. There is enough
space there and we don't really care about 32bit NUMA machines.

-Andi

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Andi Kleen]

On Wed, Mar 21, 2007 at 05:42:22PM +0200, Pekka J Enberg wrote:
> On Wed, 21 Mar 2007, Christoph Lameter wrote:
> > None of that please. The page flags already contain a node number that is 
> > accessible via page_to_nid(). Just make sure that we never put a slab onto 
> > the wrong node.
> 
> Oh well, here's a patch to find out. (I don't have a NUMA box.)

You can always use numa emulation on x86-64.  Boot with numa=fake=NODES

-Andi

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Pekka Enberg]

On 3/21/2007, "Andi Kleen" <andi@firstfloor.org> wrote:
> You can always use numa emulation on x86-64.  Boot with 
> numa=fake=NODES

Yes, I know, but I don't have a x86-64 box either =)

Re: [RFC, PATCH] SLAB : [NUMA] keep nodeid in struct page instead of struct slab

[Christoph Lameter]

On Wed, 21 Mar 2007, Andi Kleen wrote:

> On Wed, Mar 21, 2007 at 05:42:22PM +0200, Pekka J Enberg wrote:
> > On Wed, 21 Mar 2007, Christoph Lameter wrote:
> > > None of that please. The page flags already contain a node number that is 
> > > accessible via page_to_nid(). Just make sure that we never put a slab onto 
> > > the wrong node.
> > 
> > Oh well, here's a patch to find out. (I don't have a NUMA box.)
> 
> You can always use numa emulation on x86-64.  Boot with numa=fake=NODES

This wont help you to debug strange fallback scenarios on unusual NUMA 
platforms.

[RFC] NUMA : could we introduce virt_to_nid() ?

[Eric Dumazet]

Hi Andi

Checking Christoph quicklist implementation, I found the same cache miss in free() than SLAB has.

/* common implementation *
int virt_to_nid(const void *addr)
{
	struct page *page = virt_to_page(addr); 
	return page_to_nid(page);
}

On some platforms (x86_64 for example), could we have a better implementation, not accessing struct page, but using phys_to_nid() ?

Thank you

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Pekka Enberg]

On 3/23/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Checking Christoph quicklist implementation, I found the same cache miss in
> free() than SLAB has.
>
> /* common implementation *
> int virt_to_nid(const void *addr)
> {
>         struct page *page = virt_to_page(addr);
>         return page_to_nid(page);
> }
>
> On some platforms (x86_64 for example), could we have a better implementation,
> not accessing struct page, but using phys_to_nid() ?

Sounds good to me. At least cache_free_alien() in mm/slab.c to should
be converted to use it.

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Eric Dumazet]

On Fri, 23 Mar 2007 14:48:24 +0200
"Pekka Enberg" <penberg@cs.helsinki.fi> wrote:

> On 3/23/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> > Checking Christoph quicklist implementation, I found the same cache miss in
> > free() than SLAB has.
> >
> > /* common implementation *
> > int virt_to_nid(const void *addr)
> > {
> >         struct page *page = virt_to_page(addr);
> >         return page_to_nid(page);
> > }
> >
> > On some platforms (x86_64 for example), could we have a better implementation,
> > not accessing struct page, but using phys_to_nid() ?
> 
> Sounds good to me. At least cache_free_alien() in mm/slab.c to should
> be converted to use it.
> 

Not yet :(

Because in slab we currently need virt_to_slab(addr)->nodeid, not page_to_nid(virt_to_page(addr))

It might be different, according to Christoph

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Andi Kleen]

On Fri, Mar 23, 2007 at 12:15:12PM +0100, Eric Dumazet wrote:
> Hi Andi
> 
> Checking Christoph quicklist implementation, I found the same cache miss in free() than SLAB has.
> 
> /* common implementation *
> int virt_to_nid(const void *addr)
> {
> 	struct page *page = virt_to_page(addr); 
> 	return page_to_nid(page);
> }
> 
> On some platforms (x86_64 for example), could we have a better implementation, not accessing struct page, but using phys_to_nid() ?

Sure why not. Please send a patch.

-Andi

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Christoph Lameter]

On Fri, 23 Mar 2007, Eric Dumazet wrote:

> Checking Christoph quicklist implementation, I found the same cache miss in free() than SLAB has.
> 
> /* common implementation *
> int virt_to_nid(const void *addr)
> {
> 	struct page *page = virt_to_page(addr); 
> 	return page_to_nid(page);
> }
> 
> On some platforms (x86_64 for example), could we have a better 
> implementation, not accessing struct page, but using phys_to_nid() ?

This is going to pollute the caches since we then use multiple ways to 
determine the node of a page. Its better to stay with the same approach 
for all pages. The page struct is used for many other purposes as well. 
Its likely to be in the cpu cache.

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Eric Dumazet]

On Fri, 23 Mar 2007 07:50:28 -0700 (PDT)
Christoph Lameter <christoph@lameter.com> wrote:

> On Fri, 23 Mar 2007, Eric Dumazet wrote:
> 
> > Checking Christoph quicklist implementation, I found the same cache miss in free() than SLAB has.
> > 
> > /* common implementation *
> > int virt_to_nid(const void *addr)
> > {
> > 	struct page *page = virt_to_page(addr); 
> > 	return page_to_nid(page);
> > }
> > 
> > On some platforms (x86_64 for example), could we have a better 
> > implementation, not accessing struct page, but using phys_to_nid() ?
> 
> This is going to pollute the caches since we then use multiple ways to 
> determine the node of a page. Its better to stay with the same approach 
> for all pages. The page struct is used for many other purposes as well. 
> Its likely to be in the cpu cache.
> 

Sorry ? page structs are not in cpu cache at all.

phys_to_nid() on my 16 GB x86_64 machine uses one single cache line, shared by all pages.
This single cache line is cache hot yes, not "struct page"s ...

And on this machine, thats about 224 Mbytes of 'struct pages'

You carefully commented your alloc() function saying it is touching two cache lines.
But you omited to say that free() function needs 3 cache lines if CONFIG_NUMA

For SLAB use, page struct is needed because we use lru.{next|prev} to store slab/cachep pointers, but for a pure page allocator, unless I misread your patch, we dont need it, if virt_to_nid() can do its job without it.

Re: [RFC] NUMA : could we introduce virt_to_nid() ?

[Christoph Lameter]

On Fri, 23 Mar 2007, Eric Dumazet wrote:

> Sorry ? page structs are not in cpu cache at all.

They are if they are in any way handled by the VM.
 
> You carefully commented your alloc() function saying it is touching two cache lines.
> But you omited to say that free() function needs 3 cache lines if CONFIG_NUMA

Yes it needs the third cacheline to check the node of the page in the NUMA 
case.

> For SLAB use, page struct is needed because we use lru.{next|prev} to 
> store slab/cachep pointers, but for a pure page allocator, unless I 
> misread your patch, we dont need it, if virt_to_nid() can do its job 
> without it.

The page allocator is only handling page_structs.