Re: [PATCH] slub: remove one code path and reduce lock contention in __slab_free()
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()
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()
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()
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()
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()
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/