[RFC] SLUB - define OO_ macro instead of hardcoded numbers

---

Please check -- wouldn't it be better to use such a macro?
At least it makes easy to understand the 65535 constant is
coming from if only I didn't miss anything :)

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

Index: linux-2.6.git/mm/slub.c
===================================================================
--- linux-2.6.git.orig/mm/slub.c 2008-09-26 13:45:16.000000000 +0400
+++ linux-2.6.git/mm/slub.c 2008-10-22 20:10:59.000000000 +0400
@@ -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 OO_MAX OO_MASK
+
/* 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 > OO_MAX)
+ max_objects = OO_MAX;

if (page->objects != max_objects) {
slab_err(s, page, "Wrong number of objects. Found %d but "
@@ -1819,8 +1823,8 @@ static inline int slab_order(int size, i
int rem;
int min_order = slub_min_order;

- if ((PAGE_SIZE << min_order) / size > 65535)
- return get_order(size * 65535) - 1;
+ if ((PAGE_SIZE << min_order) / size > OO_MAX)
+ return get_order(size * OO_MAX) - 1;

for (order = max(min_order,
fls(min_objects * size - 1) - PAGE_SHIFT);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> Please check -- wouldn't it be better to use such a macro?[/color]

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?

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

[Christoph Lameter - Wed, Oct 22, 2008 at 09:28:14AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> Please check -- wouldn't it be better to use such a macro?[/color]
>
> 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?
>[/color]

I supposed it would mean maximum object number inside page (ie quantity) which
is happen to be the same value as OO_MASK. Maybe OO_MAX_OBJ?

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

[Cyrill Gorcunov - Wed, Oct 22, 2008 at 08:35:30PM +0400]
| [Christoph Lameter - Wed, Oct 22, 2008 at 09:28:14AM -0700]
| > On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
| >
| >> 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?
| >
|
| I supposed it would mean maximum object number inside page (ie quantity) which
| is happen to be the same value as OO_MASK. Maybe OO_MAX_OBJ?
|
| - Cyrill -

Btw Christoph fix me if I'm wrong but this 65535 is directly related to
16 bit shift. If we change the first value without changing the second we
just break the SLUB I guess. I didn't read/understand SLUB code in details
so could be wrong.

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

[Cyrill Gorcunov - Wed, Oct 22, 2008 at 08:53:54PM +0400]
| [Cyrill Gorcunov - Wed, Oct 22, 2008 at 08:35:30PM +0400]
| | [Christoph Lameter - Wed, Oct 22, 2008 at 09:28:14AM -0700]
| | > On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
| | >
| | >> 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?
| | >
| |
| | I supposed it would mean maximum object number inside page (ie quantity) which
| | is happen to be the same value as OO_MASK. Maybe OO_MAX_OBJ?
| |
| | - Cyrill -
|
| Btw Christoph fix me if I'm wrong but this 65535 is directly related to
| 16 bit shift. If we change the first value without changing the second we
| just break the SLUB I guess. I didn't read/understand SLUB code in details
| so could be wrong.
|
| - Cyrill -

Christoph how about this one?

- Cyrill -
---

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

Index: linux-2.6.git/mm/slub.c
===================================================================
--- linux-2.6.git.orig/mm/slub.c 2008-10-22 21:11:26.000000000 +0400
+++ linux-2.6.git/mm/slub.c 2008-10-22 21:19:12.000000000 +0400
@@ -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 OO_MAX_OBJS 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 > OO_MAX_OBJS)
+ max_objects = OO_MAX_OBJS;

if (page->objects != max_objects) {
slab_err(s, page, "Wrong number of objects. Found %d but "
@@ -1819,8 +1823,8 @@ static inline int slab_order(int size, i
int rem;
int min_order = slub_min_order;

- if ((PAGE_SIZE << min_order) / size > 65535)
- return get_order(size * 65535) - 1;
+ if ((PAGE_SIZE << min_order) / size > OO_MAX_OBJS)
+ return get_order(size * OO_MAX_OBJS) - 1;

for (order = max(min_order,
fls(min_objects * size - 1) - PAGE_SHIFT);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

[Christoph Lameter - Wed, Oct 22, 2008 at 10:47:09AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> Christoph how about this one?[/color]
>
> 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?
>[/color]

If name is not that good -- maybe OO_OBJS_PER_PAGE? :)

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

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> Christoph how about this one?[/color]

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?

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

[Pekka Enberg - Wed, Oct 22, 2008 at 08:50:56PM +0300][color=blue]
> Christoph Lameter wrote:[color=green]
>> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>>[color=darkred]
>>> Christoph how about this one?[/color]
>>
>> 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?[/color]
>
> Maybe call the page->objects maximum MAX_OBJS_PER_PAGE as it's not
> strictly related to the other OO code?
>
> Pekka
>[/color]

Something like that?

- Cyrill -
---

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

Index: linux-2.6.git/mm/slub.c
===================================================================
--- linux-2.6.git.orig/mm/slub.c 2008-10-22 21:11:26.000000000 +0400
+++ linux-2.6.git/mm/slub.c 2008-10-22 21:57:11.000000000 +0400
@@ -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 "
@@ -1819,8 +1823,8 @@ static inline int slab_order(int size, i
int rem;
int min_order = slub_min_order;

- if ((PAGE_SIZE << min_order) / size > 65535)
- return get_order(size * 65535) - 1;
+ if ((PAGE_SIZE << min_order) / size > MAX_OBJS_PER_PAGE)
+ return get_order(size * MAX_OBJS_PER_PAGE) - 1;

for (order = max(min_order,
fls(min_objects * size - 1) - PAGE_SHIFT);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Christoph Lameter wrote:[color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> Christoph how about this one?[/color]
>
> 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?[/color]

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

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

Cyrill Gorcunov wrote:[color=blue]
> Something like that?
>
> - Cyrill -
> ---
>
> mm/slub.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> Index: linux-2.6.git/mm/slub.c
> ===================================================================
> --- linux-2.6.git.orig/mm/slub.c 2008-10-22 21:11:26.000000000 +0400
> +++ linux-2.6.git/mm/slub.c 2008-10-22 21:57:11.000000000 +0400
> @@ -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 "
> @@ -1819,8 +1823,8 @@ static inline int slab_order(int size, i
> int rem;
> int min_order = slub_min_order;
>
> - if ((PAGE_SIZE << min_order) / size > 65535)
> - return get_order(size * 65535) - 1;
> + if ((PAGE_SIZE << min_order) / size > MAX_OBJS_PER_PAGE)
> + return get_order(size * MAX_OBJS_PER_PAGE) - 1;
>
> for (order = max(min_order,
> fls(min_objects * size - 1) - PAGE_SHIFT);[/color]

Looks good to me. If Christoph ACKs this, please send me a proper patch
with your sign-off and I'll merge it to slab.git.

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

On Wed, 22 Oct 2008, Pekka Enberg wrote:
[color=blue]
> Maybe call the page->objects maximum MAX_OBJS_PER_PAGE as it's not strictly
> related to the other OO code?[/color]

Yes. Sounds good.

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

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> +#define OO_SHIFT 16
> +#define OO_MASK ((1 << OO_SHIFT) - 1)
> +#define MAX_OBJS_PER_PAGE 65535 /* see struct page.objects */[/color]

This is == OO_MASK right? Otherwise things will break when we change
OO_SHIFT.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

[Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> +#define OO_SHIFT 16
>> +#define OO_MASK ((1 << OO_SHIFT) - 1)
>> +#define MAX_OBJS_PER_PAGE 65535 /* see struct page.objects */[/color]
>
> This is == OO_MASK right? Otherwise things will break when we change
> OO_SHIFT.
>[/color]

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 :)

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

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> [Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700][color=green]
>> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>>[color=darkred]
>>> +#define OO_SHIFT 16
>>> +#define OO_MASK ((1 << OO_SHIFT) - 1)
>>> +#define MAX_OBJS_PER_PAGE 65535 /* see struct page.objects */[/color]
>>
>> This is == OO_MASK right? Otherwise things will break when we change
>> OO_SHIFT.
>>[/color]
>
> 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 :)[/color]

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)

?


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

[Christoph Lameter - Wed, Oct 22, 2008 at 11:24:58AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> [Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700][color=darkred]
>>> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>>>
>>>> +#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.
>>>[/color]
>>
>> 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 :)[/color]
>
> 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)
>
> ?
>
>[/color]

Looks really good for me (if it worth anything). But Christoph
doesn't OO_**** 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?

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

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> Looks really good for me (if it worth anything). But Christoph
> doesn't OO_**** 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?[/color]


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.

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

[Christoph Lameter - Wed, Oct 22, 2008 at 11:24:58AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> [Christoph Lameter - Wed, Oct 22, 2008 at 11:10:56AM -0700][color=darkred]
>>> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>>>
>>>> +#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.
>>>[/color]
>>
>> 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 :)[/color]
>
> 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)
>
> ?
>
>[/color]

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 */

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

[Christoph Lameter - Wed, Oct 22, 2008 at 11:45:55AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> Looks really good for me (if it worth anything). But Christoph
>> doesn't OO_**** 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?[/color]
>
>
> 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.
>[/color]

Ah... I see. So wouldn't you mind to just mentoin page.objects in comment
like 'since page.objects is u16' instead of bits_in magic? Anyone who
will (if any) changing page structure is to grep the sources and find
this comment and will fix MAX_OBJS_PER_PAGE definition.

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

On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
[color=blue]
> 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 */[/color]

Ok.

Acked-by: Christoph Lameter <cl@linux-foundation.org>

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

[Christoph Lameter - Wed, Oct 22, 2008 at 11:49:56AM -0700][color=blue]
> On Wed, 22 Oct 2008, Cyrill Gorcunov wrote:
>[color=green]
>> 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 */[/color]
>
> Ok.
>
> Acked-by: Christoph Lameter <cl@linux-foundation.org>
>[/color]

Thanks for review -- will make normal patch shortly and
send it to Pekka. Thanks!

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