SLUB - define OO_ macro instead of hardcoded numbers

Please check -- wouldn't it be better to use such a macro?

Quoted 2 lines

Please check -- wouldn't it be better to use such a macro?

Looks good. But could you rename OO_MAX to something different? There is already s->max which may cause confusion because s->max is the maximum number of objects in a slab. OO_MAX is the maximum mask?

Christoph how about this one?

Quoted 1 line · Cyrill Gorcunov

Christoph how about this one?

Ok. Looks a bit better but we still have two maxes here

s->max which refers to the maximum number of objects per slab page for a specific slab cache (depends on the runtime configuration). OO_MAX_OBJS refers to the maximum number of objects per slab page that any slab cache can be configured for which is a compile time limit.

Maybe this is okay, Pekka?

Quoted 11 lines · Christoph Lameter

Quoted 1 line · Cyrill Gorcunov

Christoph how about this one?

Ok. Looks a bit better but we still have two maxes here

s->max which refers to the maximum number of objects per slab page for a specific slab cache (depends on the runtime configuration). OO_MAX_OBJS refers to the maximum number of objects per slab page that any slab cache can be configured for which is a compile time limit.

Quoted 1 line

Maybe this is okay, Pekka?

Maybe call the page->objects maximum MAX_OBJS_PER_PAGE as it's not strictly related to the other OO code?

Pekka

Something like that?

- Cyrill -
---

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

Index: linux-2.6.git/mm/slub.c
===================================================================

mm/slub.c +9 -5

@@ -153,6 +153,10 @@
 #define ARCH_SLAB_MINALIGN __alignof__(unsigned long long)
 #endif
 
+#define OO_SHIFT	16
+#define OO_MASK		((1 << OO_SHIFT) - 1)
+#define MAX_OBJS_PER_PAGE	65535 /* see struct page.objects */
+
 /* Internal SLUB flags */
 #define __OBJECT_POISON		0x80000000 /* Poison object */
 #define __SYSFS_ADD_DEFERRED	0x40000000 /* Not yet visible via sysfs */

@@ -290,7 +294,7 @@ static inline struct kmem_cache_order_ob
 						unsigned long size)
 {
 	struct kmem_cache_order_objects x = {
-		(order << 16) + (PAGE_SIZE << order) / size
+		(order << OO_SHIFT) + (PAGE_SIZE << order) / size
 	};
 
 	return x;

@@ -298,12 +302,12 @@ static inline struct kmem_cache_order_ob
 
 static inline int oo_order(struct kmem_cache_order_objects x)
 {
-	return x.x >> 16;
+	return x.x >> OO_SHIFT;
 }
 
 static inline int oo_objects(struct kmem_cache_order_objects x)
 {
-	return x.x & ((1 << 16) - 1);
+	return x.x & OO_MASK;
 }
 
 #ifdef CONFIG_SLUB_DEBUG

@@ -764,8 +768,8 @@ static int on_freelist(struct kmem_cache
 	}
 
 	max_objects = (PAGE_SIZE << compound_order(page)) / s->size;
-	if (max_objects > 65535)
-		max_objects = 65535;
+	if (max_objects > MAX_OBJS_PER_PAGE)
+		max_objects = MAX_OBJS_PER_PAGE;
 
 	if (page->objects != max_objects) {
 		slab_err(s, page, "Wrong number of objects. Found %d but "

mm/slub.c line 158

Quoted 4 lines

mm/slub.c +3 -0

+#define OO_SHIFT	16
+#define OO_MASK		((1 << OO_SHIFT) - 1)
+#define MAX_OBJS_PER_PAGE	65535 /* see struct page.objects */

This is == OO_MASK right? Otherwise things will break when we change OO_SHIFT.

[Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700]

Quoted 9 lines · Cyrill Gorcunov

mm/slub.c line 158

Quoted 4 lines

mm/slub.c +3 -0

+#define OO_SHIFT	16
+#define OO_MASK		((1 << OO_SHIFT) - 1)
+#define MAX_OBJS_PER_PAGE	65535 /* see struct page.objects */

This is == OO_MASK right? Otherwise things will break when we change OO_SHIFT.

Yes, I set it 65535 directly to distinguish it from OO_MASK meaning not value and point to page.objects since they are u16. Which meant that is the point where all limits start. So it we set OO_SHIFT to say 14 it will not be a problem but if we exceed 16 bits it will break SLUB. Am I right? (I become scratching the head :)

Quoted 12 lines

[Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700]

Quoted 9 lines · Cyrill Gorcunov

mm/slub.c line 158

Quoted 4 lines

mm/slub.c +3 -0

+#define OO_SHIFT	16
+#define OO_MASK		((1 << OO_SHIFT) - 1)
+#define MAX_OBJS_PER_PAGE	65535 /* see struct page.objects */

This is == OO_MASK right? Otherwise things will break when we change OO_SHIFT.

Quoted 6 lines

Yes, I set it 65535 directly to distinguish it from OO_MASK meaning not value and point to page.objects since they are u16. Which meant that is the point where all limits start. So it we set OO_SHIFT to say 14 it will not be a problem but if we exceed 16 bits it will break SLUB. Am I right? (I become scratching the head :)

If you set it > 16 then the size of the field in struct page is violated.

So

#define MAX_OBJ_PER_PAGE MIN(1 << bits_in(page.objects) - 1, OO_MASK)

?

Looks really good for me (if it worth anything). But Christoph doesn't OO_SHIT inspired by u16 too which means we could use MAX_OBJ_PER_PAGE in form you mentoined but maybe we should define

#define OO_SHIFT bits_in(page.objects) to point out why we use 16 not 14, not 18 or whatever? How do you think?

Quoted 5 lines

Looks really good for me (if it worth anything). But Christoph doesn't OO_SHIT inspired by u16 too which means we could use MAX_OBJ_PER_PAGE in form you mentoined but maybe we should define

Quoted 2 lines

#define OO_SHIFT bits_in(page.objects) to point out why we use 16 not 14, not 18 or whatever? How do you think?

The choice of the bit size in page.objects is determined by the available bytes there. The choice of the OO_SHIFT (nice typo there) is determined by the use of a 32bit int that we want to cut into two halves.

Quoted 12 lines

[Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700]

Quoted 9 lines · Cyrill Gorcunov

mm/slub.c line 158

Quoted 4 lines

mm/slub.c +3 -0

+#define OO_SHIFT	16
+#define OO_MASK		((1 << OO_SHIFT) - 1)
+#define MAX_OBJS_PER_PAGE	65535 /* see struct page.objects */

This is == OO_MASK right? Otherwise things will break when we change OO_SHIFT.

Quoted 6 lines

Yes, I set it 65535 directly to distinguish it from OO_MASK meaning not value and point to page.objects since they are u16. Which meant that is the point where all limits start. So it we set OO_SHIFT to say 14 it will not be a problem but if we exceed 16 bits it will break SLUB. Am I right? (I become scratching the head :)

If you set it > 16 then the size of the field in struct page is violated.

So

#define MAX_OBJ_PER_PAGE MIN(1 << bits_in(page.objects) - 1, OO_MASK)

?

I think the patch becomes more complicated as it should be. Maybe just use:

#define MAX_OBJ_PER_PAGE 65535 /* since page.objects is u16 */

Quoted 5 lines

I think the patch becomes more complicated as it should be. Maybe just use:

#define MAX_OBJ_PER_PAGE 65535 /* since page.objects is u16 */

Ok.

Acked-by Christoph Lameter <[email protected]>

Maybe call the page->objects maximum MAX_OBJS_PER_PAGE as it's not strictly related to the other OO code?

Quoted 2 lines

Christoph how about this one?

Ok. Looks a bit better but we still have two maxes here

s->max which refers to the maximum number of objects per slab page for a specific slab cache (depends on the runtime configuration). OO_MAX_OBJS refers to the maximum number of objects per slab page that any slab cache can be configured for which is a compile time limit.

Maybe this is okay, Pekka?