Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[Christoph Lameter]

On Sat, 28 Jul 2012, JoonSoo Kim wrote:

> 2012/7/28 Christoph Lameter :
> > On Sat, 28 Jul 2012, Joonsoo Kim wrote:
> >
> >> Subject and commit log are changed from v1.
> >
> > That looks a bit better. But the changelog could use more cleanup and
> > clearer expression.
> >
> >> @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, 
> >> struct page *page,
> >>  return;
> >>  }
> >>
> >> + if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
> >> + goto slab_empty;
> >> +
> >
> > So we can never encounter a empty slab that was frozen before? Really?
>
> In my suggestion,  'was_frozen = 1' is "always" handled without taking a lock.

Yepo that is true with this patch.

> Then, never hit following code.
> + if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
> + goto slab_empty;
> +


Correct.

> Instead, hit following code.
> if (likely(!n)) {
>
> /*
>  * If we just froze the page then put it onto the
>  * per cpu partial list.
>  */
> if (new.frozen && !was_frozen) {
> put_cpu_partial(s, page, 1);
> stat(s, CPU_PARTIAL_FREE);
> }
> /*
>  * The list lock was not taken therefore no list
>  * activity can be necessary.
>  */
> if (was_frozen)
> stat(s, FREE_FROZEN);
> return;
> }
>
> So, even if we encounter a empty slab that was frozen before, we just
> do "stat(s, FREE_FROZEN)".
> Please let me know my answer is sufficient.

Yes.

Acked-by: Christoph Lameter 

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

Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[Christoph Lameter]

On Sat, 28 Jul 2012, JoonSoo Kim wrote:

 2012/7/28 Christoph Lameter c...@linux.com:
  On Sat, 28 Jul 2012, Joonsoo Kim wrote:
 
  Subject and commit log are changed from v1.
 
  That looks a bit better. But the changelog could use more cleanup and
  clearer expression.
 
  @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, 
  struct page *page,
   return;
   }
 
  + if (unlikely(!new.inuse  n-nr_partial  s-min_partial))
  + goto slab_empty;
  +
 
  So we can never encounter a empty slab that was frozen before? Really?

 In my suggestion,  'was_frozen = 1' is always handled without taking a lock.

Yepo that is true with this patch.

 Then, never hit following code.
 + if (unlikely(!new.inuse  n-nr_partial  s-min_partial))
 + goto slab_empty;
 +


Correct.

 Instead, hit following code.
 if (likely(!n)) {

 /*
  * If we just froze the page then put it onto the
  * per cpu partial list.
  */
 if (new.frozen  !was_frozen) {
 put_cpu_partial(s, page, 1);
 stat(s, CPU_PARTIAL_FREE);
 }
 /*
  * The list lock was not taken therefore no list
  * activity can be necessary.
  */
 if (was_frozen)
 stat(s, FREE_FROZEN);
 return;
 }

 So, even if we encounter a empty slab that was frozen before, we just
 do stat(s, FREE_FROZEN).
 Please let me know my answer is sufficient.

Yes.

Acked-by: Christoph Lameter c...@linux.com

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

Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[JoonSoo Kim]

2012/7/28 Christoph Lameter :
> On Sat, 28 Jul 2012, Joonsoo Kim wrote:
>
>> Subject and commit log are changed from v1.
>
> That looks a bit better. But the changelog could use more cleanup and
> clearer expression.
>
>> @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, struct 
>> page *page,
>>  return;
>>  }
>>
>> + if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
>> + goto slab_empty;
>> +
>
> So we can never encounter a empty slab that was frozen before? Really?

In my suggestion,  'was_frozen = 1' is "always" handled without taking a lock.
Then, never hit following code.
+ if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
+ goto slab_empty;
+

Instead, hit following code.
if (likely(!n)) {

/*
 * If we just froze the page then put it onto the
 * per cpu partial list.
 */
if (new.frozen && !was_frozen) {
put_cpu_partial(s, page, 1);
stat(s, CPU_PARTIAL_FREE);
}
/*
 * The list lock was not taken therefore no list
 * activity can be necessary.
 */
if (was_frozen)
stat(s, FREE_FROZEN);
return;
}

So, even if we encounter a empty slab that was frozen before, we just
do "stat(s, FREE_FROZEN)".
Please let me know my answer is sufficient.
Thanks!!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[JoonSoo Kim]

2012/7/28 Christoph Lameter c...@linux.com:
 On Sat, 28 Jul 2012, Joonsoo Kim wrote:

 Subject and commit log are changed from v1.

 That looks a bit better. But the changelog could use more cleanup and
 clearer expression.

 @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, struct 
 page *page,
  return;
  }

 + if (unlikely(!new.inuse  n-nr_partial  s-min_partial))
 + goto slab_empty;
 +

 So we can never encounter a empty slab that was frozen before? Really?

In my suggestion,  'was_frozen = 1' is always handled without taking a lock.
Then, never hit following code.
+ if (unlikely(!new.inuse  n-nr_partial  s-min_partial))
+ goto slab_empty;
+

Instead, hit following code.
if (likely(!n)) {

/*
 * If we just froze the page then put it onto the
 * per cpu partial list.
 */
if (new.frozen  !was_frozen) {
put_cpu_partial(s, page, 1);
stat(s, CPU_PARTIAL_FREE);
}
/*
 * The list lock was not taken therefore no list
 * activity can be necessary.
 */
if (was_frozen)
stat(s, FREE_FROZEN);
return;
}

So, even if we encounter a empty slab that was frozen before, we just
do stat(s, FREE_FROZEN).
Please let me know my answer is sufficient.
Thanks!!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[Christoph Lameter]

On Sat, 28 Jul 2012, Joonsoo Kim wrote:

> Subject and commit log are changed from v1.

That looks a bit better. But the changelog could use more cleanup and
clearer expression.

> @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, struct 
> page *page,
>  return;
>  }
>
> + if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
> + goto slab_empty;
> +

So we can never encounter a empty slab that was frozen before? Really?

Remote frees can decrement inuse again. All objects of a slab frozen on
one cpu could be allocated while the slab is still frozen. The
unfreezing requires slab_alloc to encounter a NULL pointer after all.

A remote processor could obtain a pointer to all these objects and free
them. The code here would cause an unfreeze action. Another alloc on the
first processor would cause a *second* unfreeze action on a page that was
freed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()

[Christoph Lameter]

On Sat, 28 Jul 2012, Joonsoo Kim wrote:

 Subject and commit log are changed from v1.

That looks a bit better. But the changelog could use more cleanup and
clearer expression.

 @@ -2490,25 +2492,17 @@ static void __slab_free(struct kmem_cache *s, struct 
 page *page,
  return;
  }

 + if (unlikely(!new.inuse  n-nr_partial  s-min_partial))
 + goto slab_empty;
 +

So we can never encounter a empty slab that was frozen before? Really?

Remote frees can decrement inuse again. All objects of a slab frozen on
one cpu could be allocated while the slab is still frozen. The
unfreezing requires slab_alloc to encounter a NULL pointer after all.

A remote processor could obtain a pointer to all these objects and free
them. The code here would cause an unfreeze action. Another alloc on the
first processor would cause a *second* unfreeze action on a page that was
freed.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/