[PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Joonsoo Kim]

s->cpu_partial determine the maximum number of objects kept
in the per cpu partial lists of a processor. Currently, it is used for
not only per cpu partial list but also cpu freelist. Therefore
get_partial_node() doesn't work properly according to our first intention.

Fix it as forcibly assigning 0 to objects count when we get for cpu freelist.

Signed-off-by: Joonsoo Kim <js1304@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>

diff --git a/mm/slub.c b/mm/slub.c
index efce427..88dca1d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1550,7 +1550,12 @@ static void *get_partial_node(struct kmem_cache *s,
 			c->page = page;
 			stat(s, ALLOC_FROM_PARTIAL);
 			object = t;
-			available =  page->objects - page->inuse;
+
+			/*
+			 * We don't want to stop without trying to get
+			 * cpu partial slab. So, forcibly set 0 to available
+			 */
+			available = 0;
 		} else {
 			available = put_cpu_partial(s, page, 0);
 			stat(s, CPU_PARTIAL_NODE);
-- 
1.7.9.5

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Christoph Lameter]

On Thu, 16 Aug 2012, Joonsoo Kim wrote:

> s->cpu_partial determine the maximum number of objects kept
> in the per cpu partial lists of a processor. Currently, it is used for
> not only per cpu partial list but also cpu freelist. Therefore
> get_partial_node() doesn't work properly according to our first intention.

The "cpu freelist" in slub is the number of free objects in a specific
page. There is nothing that s->cpu_partial can do about that.

Maybe I do not understand you correctly. Could you explain this in some
more detail?

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[JoonSoo Kim]

2012/8/16 Christoph Lameter <cl@linux.com>:
> On Thu, 16 Aug 2012, Joonsoo Kim wrote:
>
>> s->cpu_partial determine the maximum number of objects kept
>> in the per cpu partial lists of a processor. Currently, it is used for
>> not only per cpu partial list but also cpu freelist. Therefore
>> get_partial_node() doesn't work properly according to our first intention.
>
> The "cpu freelist" in slub is the number of free objects in a specific
> page. There is nothing that s->cpu_partial can do about that.
>
> Maybe I do not understand you correctly. Could you explain this in some
> more detail?

I assume that cpu slab and cpu partial slab are not same thing.

In my definition,
cpu slab is in c->page,
cpu partial slab is in c->partial

When we have no free objects in cpu slab and cpu partial slab, we try
to get slab via get_partial_node().
In that function, we call acquire_slab(). Then we hit "!object" case
(for cpu slab).
In that case, we test available with s->cpu_partial.

I think that s->cpu_partial is for cpu partial slab, not cpu slab.
So this test is not proper.
This patch is for correcting this.

Thanks!

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Christoph Lameter]

On Thu, 16 Aug 2012, JoonSoo Kim wrote:

> > Maybe I do not understand you correctly. Could you explain this in some
> > more detail?
>
> I assume that cpu slab and cpu partial slab are not same thing.
>
> In my definition,
> cpu slab is in c->page,
> cpu partial slab is in c->partial

Correct.

> When we have no free objects in cpu slab and cpu partial slab, we try
> to get slab via get_partial_node().
> In that function, we call acquire_slab(). Then we hit "!object" case
> (for cpu slab).
> In that case, we test available with s->cpu_partial.

> I think that s->cpu_partial is for cpu partial slab, not cpu slab.

Ummm... Not entirely. s->cpu_partial is the mininum number of objects to
"cache" per processor. This includes the objects available in the per cpu
slab and the other slabs on the per cpu partial list.

> So this test is not proper.

Ok so this tests occurs in get_partial_node() not in acquire_slab().

If object == NULL then we have so far nothing allocated an c->page ==
NULL. The first allocation refills the cpu_slab (by freezing a slab) so
that we can allocate again. If we go through the loop again then we refill
the per cpu partial lists with more frozen slabs until we have a
sufficient number of objects that we can allocate without obtaining any
locks.

> This patch is for correcting this.

There is nothing wrong with this. The name c->cpu_partial is a bit
awkward. Maybe rename that to c->min_per_cpu_objects or so?

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[JoonSoo Kim]

>> I think that s->cpu_partial is for cpu partial slab, not cpu slab.
>
> Ummm... Not entirely. s->cpu_partial is the mininum number of objects to
> "cache" per processor. This includes the objects available in the per cpu
> slab and the other slabs on the per cpu partial list.

Hmm..
When we do test for unfreezing in put_cpu_partial(), we only compare
how many objects is in "cpu partial slab" with s->cpu_partial,
although it is just approximation of number of objects kept in cpu partial slab.
We do not consider number of objects kept in cpu slab in that time.
This makes me "s->cpu_partial is only for cpu partial slab, not cpu slab".

We can't count number of objects kept in in cpu slab easily.
Therefore, it it more consistent that s->cpu_partial is always for cpu
partial slab.

But, if you prefer that s->cpu_partial is for both cpu slab and cpu
partial slab,
get_partial_node() needs an another minor fix.
We should add number of objects in cpu slab when we refill cpu partial slab.
Following is my suggestion.

@@ -1546,7 +1546,7 @@ static void *get_partial_node(struct kmem_cache *s,
        spin_lock(&n->list_lock);
        list_for_each_entry_safe(page, page2, &n->partial, lru) {
                void *t = acquire_slab(s, n, page, object == NULL);
-               int available;
+               int available, nr = 0;

                if (!t)
                        break;
@@ -1557,10 +1557,10 @@ static void *get_partial_node(struct kmem_cache *s,
                        object = t;
                        available =  page->objects - page->inuse;
                } else {
-                       available = put_cpu_partial(s, page, 0);
+                       nr = put_cpu_partial(s, page, 0);
                        stat(s, CPU_PARTIAL_NODE);
                }
-               if (kmem_cache_debug(s) || available > s->cpu_partial / 2)
+               if (kmem_cache_debug(s) || (available + nr) >
s->cpu_partial / 2)
                        break;

        }

If you agree with this suggestion, I send a patch for this.


> If object == NULL then we have so far nothing allocated an c->page ==
> NULL. The first allocation refills the cpu_slab (by freezing a slab) so
> that we can allocate again. If we go through the loop again then we refill
> the per cpu partial lists with more frozen slabs until we have a
> sufficient number of objects that we can allocate without obtaining any
> locks.
>
>> This patch is for correcting this.
>
> There is nothing wrong with this. The name c->cpu_partial is a bit
> awkward. Maybe rename that to c->min_per_cpu_objects or so?

Okay.
It look better.

Thanks!

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Christoph Lameter]

On Thu, 16 Aug 2012, JoonSoo Kim wrote:

> But, if you prefer that s->cpu_partial is for both cpu slab and cpu
> partial slab,
> get_partial_node() needs an another minor fix.
> We should add number of objects in cpu slab when we refill cpu partial slab.
> Following is my suggestion.
>
> @@ -1546,7 +1546,7 @@ static void *get_partial_node(struct kmem_cache *s,
>         spin_lock(&n->list_lock);
>         list_for_each_entry_safe(page, page2, &n->partial, lru) {
>                 void *t = acquire_slab(s, n, page, object == NULL);
> -               int available;
> +               int available, nr = 0;
>
>                 if (!t)
>                         break;
> @@ -1557,10 +1557,10 @@ static void *get_partial_node(struct kmem_cache *s,
>                         object = t;
>                         available =  page->objects - page->inuse;
>                 } else {
> -                       available = put_cpu_partial(s, page, 0);
> +                       nr = put_cpu_partial(s, page, 0);
>                         stat(s, CPU_PARTIAL_NODE);
>                 }
> -               if (kmem_cache_debug(s) || available > s->cpu_partial / 2)
> +               if (kmem_cache_debug(s) || (available + nr) >
> s->cpu_partial / 2)
>                         break;
>
>         }
>
> If you agree with this suggestion, I send a patch for this.

What difference does this patch make? At the end of the day you need the
total number of objects available in the partial slabs and the cpu slab
for comparison.

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[JoonSoo Kim]

2012/8/17 Christoph Lameter <cl@linux.com>:
> On Thu, 16 Aug 2012, JoonSoo Kim wrote:
>
>> But, if you prefer that s->cpu_partial is for both cpu slab and cpu
>> partial slab,
>> get_partial_node() needs an another minor fix.
>> We should add number of objects in cpu slab when we refill cpu partial slab.
>> Following is my suggestion.
>>
>> @@ -1546,7 +1546,7 @@ static void *get_partial_node(struct kmem_cache *s,
>>         spin_lock(&n->list_lock);
>>         list_for_each_entry_safe(page, page2, &n->partial, lru) {
>>                 void *t = acquire_slab(s, n, page, object == NULL);
>> -               int available;
>> +               int available, nr = 0;
>>
>>                 if (!t)
>>                         break;
>> @@ -1557,10 +1557,10 @@ static void *get_partial_node(struct kmem_cache *s,
>>                         object = t;
>>                         available =  page->objects - page->inuse;
>>                 } else {
>> -                       available = put_cpu_partial(s, page, 0);
>> +                       nr = put_cpu_partial(s, page, 0);
>>                         stat(s, CPU_PARTIAL_NODE);
>>                 }
>> -               if (kmem_cache_debug(s) || available > s->cpu_partial / 2)
>> +               if (kmem_cache_debug(s) || (available + nr) >
>> s->cpu_partial / 2)
>>                         break;
>>
>>         }
>>
>> If you agree with this suggestion, I send a patch for this.
>
> What difference does this patch make? At the end of the day you need the
> total number of objects available in the partial slabs and the cpu slab
> for comparison.

It doesn't induce any large difference, but this makes code robust and
consistent.
Consistent code make us easily knowing what code does.

It is somewhat odd that in first loop, we consider number of objects
kept in cpu slab,
but second loop exclude that number and just consider number of
objects in cpu partial slab.

Thanks!

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Christoph Lameter]

On Fri, 17 Aug 2012, JoonSoo Kim wrote:

> > What difference does this patch make? At the end of the day you need the
> > total number of objects available in the partial slabs and the cpu slab
> > for comparison.
>
> It doesn't induce any large difference, but this makes code robust and
> consistent.
> Consistent code make us easily knowing what code does.

Consistency depends on the way you think about the code.

> It is somewhat odd that in first loop, we consider number of objects
> kept in cpu slab,
> but second loop exclude that number and just consider number of
> objects in cpu partial slab.

In the loop we consider the number of objects available to the cpu
without locking.

First we populate the per_cpu slab and if that does not give us enough per
cpu objects then we use the per cpu partial list to increase that number
to the desired count given by s->cpu_partial.

"available" is the number of objects available for a particular cpu
without having to go to the partial slab lists (which means having to acquire a
per node lock).

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[JoonSoo Kim]

2012/8/17 Christoph Lameter <cl@linux.com>:
> On Fri, 17 Aug 2012, JoonSoo Kim wrote:
>
>> > What difference does this patch make? At the end of the day you need the
>> > total number of objects available in the partial slabs and the cpu slab
>> > for comparison.
>>
>> It doesn't induce any large difference, but this makes code robust and
>> consistent.
>> Consistent code make us easily knowing what code does.
>
> Consistency depends on the way you think about the code.
>
>> It is somewhat odd that in first loop, we consider number of objects
>> kept in cpu slab,
>> but second loop exclude that number and just consider number of
>> objects in cpu partial slab.
>
> In the loop we consider the number of objects available to the cpu
> without locking.
>
> First we populate the per_cpu slab and if that does not give us enough per
> cpu objects then we use the per cpu partial list to increase that number
> to the desired count given by s->cpu_partial.
>
> "available" is the number of objects available for a particular cpu
> without having to go to the partial slab lists (which means having to acquire a
> per node lock).
>

Yes! You are right!
But, currently, "available" is not used as above meaning exactly.
It is used twice and each one has different meaning.

                if (!object) {
                        c->page = page;
                        stat(s, ALLOC_FROM_PARTIAL);
                        object = t;
                        available =  page->objects - page->inuse;
                } else {
                        available = put_cpu_partial(s, page, 0);
                        stat(s, CPU_PARTIAL_NODE);
                }

See above code.
In case of !object (available =  page->objects - page->inuse;),
"available" means the number of objects in cpu slab.
In this time, we don't have any cpu partial slab, so "available" imply
the number of objects available to the cpu without locking.
This is what we want.


But, see another "available" (available = put_cpu_partial(s, page, 0);).

This "available" doesn't include the number of objects in cpu slab.
It only include the number of objects in cpu partial slab.
So, it doesn't imply the number of objects available to the cpu without locking.
This isn't what we want.

Therefore, I think a minor fix is needed for consistency.
Isn't it reasonable?

Thanks!

Re: [PATCH] slub: try to get cpu partial slab even if we get enough objects for cpu freelist

[Christoph Lameter]

On Fri, 17 Aug 2012, JoonSoo Kim wrote:

> In case of !object (available =  page->objects - page->inuse;),
> "available" means the number of objects in cpu slab.

Right because we do not have allocated any cpu partial slabs yet.

> In this time, we don't have any cpu partial slab, so "available" imply
> the number of objects available to the cpu without locking.
> This is what we want.
>
>
> But, see another "available" (available = put_cpu_partial(s, page, 0);).
>
> This "available" doesn't include the number of objects in cpu slab.

Ok. Now I see.

> Therefore, I think a minor fix is needed for consistency.
> Isn't it reasonable?

Yup it is. Let me look over your patch again.

Ok so use meaningful names for the variables to clarify the issue.

cpu_objects and partial_objects or so?

Then the check would be as you proposed in the last message

if (cpu_objects + partial_objects < s->cpu_partial ...