Re: [slubllv6 07/17] slub: explicit list_lock taking

From: David Rientjes
Date: Fri May 27 2011 - 15:40:26 EST


On Thu, 26 May 2011, Christoph Lameter wrote:

> The allocator fastpath rework does change the usage of the list_lock.
> Remove the list_lock processing from the functions that hide them from the
> critical sections and move them into those critical sections.
>

Sounds good, but I'd suggest adding comments to the now list_lock-free
functions that specify that the lock must be held (when
slab_state != DOWN).

> This in turn simplifies the support functions (no __ variant needed anymore)
> and simplifies the lock handling on bootstrap.
>

Please also specify that you're inlining add_partial().

> Signed-off-by: Christoph Lameter <cl@xxxxxxxxx>
>
> ---
> mm/slub.c | 74 ++++++++++++++++++++++++++++++--------------------------------
> 1 file changed, 36 insertions(+), 38 deletions(-)
>
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c 2011-05-24 09:40:43.764875120 -0500
> +++ linux-2.6/mm/slub.c 2011-05-24 09:40:44.814875113 -0500
> @@ -917,25 +917,21 @@ static inline void slab_free_hook(struct
> /*
> * Tracking of fully allocated slabs for debugging purposes.
> */
> -static void add_full(struct kmem_cache_node *n, struct page *page)
> +static void add_full(struct kmem_cache *s,
> + struct kmem_cache_node *n, struct page *page)
> {
> - spin_lock(&n->list_lock);
> + if (!(s->flags & SLAB_STORE_USER))
> + return;
> +
> list_add(&page->lru, &n->full);
> - spin_unlock(&n->list_lock);
> }
>
> static void remove_full(struct kmem_cache *s, struct page *page)
> {
> - struct kmem_cache_node *n;
> -
> if (!(s->flags & SLAB_STORE_USER))
> return;
>
> - n = get_node(s, page_to_nid(page));
> -
> - spin_lock(&n->list_lock);
> list_del(&page->lru);
> - spin_unlock(&n->list_lock);
> }
>
> /* Tracking of the number of slabs for debugging purposes */
> @@ -1060,8 +1056,13 @@ static noinline int free_debug_processin
> }
>
> /* Special debug activities for freeing objects */
> - if (!page->frozen && !page->freelist)
> + if (!page->frozen && !page->freelist) {
> + struct kmem_cache_node *n = get_node(s, page_to_nid(page));
> +
> + spin_lock(&n->list_lock);
> remove_full(s, page);
> + spin_unlock(&n->list_lock);
> + }
> if (s->flags & SLAB_STORE_USER)
> set_track(s, object, TRACK_FREE, addr);
> trace(s, page, object, 0);
> @@ -1420,36 +1421,26 @@ static __always_inline int slab_trylock(
> /*
> * Management of partially allocated slabs
> */
> -static void add_partial(struct kmem_cache_node *n,
> +static inline void add_partial(struct kmem_cache_node *n,
> struct page *page, int tail)
> {
> - spin_lock(&n->list_lock);
> n->nr_partial++;
> if (tail)
> list_add_tail(&page->lru, &n->partial);
> else
> list_add(&page->lru, &n->partial);
> - spin_unlock(&n->list_lock);
> }
>
> -static inline void __remove_partial(struct kmem_cache_node *n,
> +static inline void remove_partial(struct kmem_cache_node *n,
> struct page *page)
> {
> list_del(&page->lru);
> n->nr_partial--;
> }
>
> -static void remove_partial(struct kmem_cache *s, struct page *page)
> -{
> - struct kmem_cache_node *n = get_node(s, page_to_nid(page));
> -
> - spin_lock(&n->list_lock);
> - __remove_partial(n, page);
> - spin_unlock(&n->list_lock);
> -}
> -
> /*
> - * Lock slab and remove from the partial list.
> + * Lock slab, remove from the partial list and put the object into the
> + * per cpu freelist.
> *
> * Must hold list_lock.
> */

Comment looks like it got updated prematurely in this patch.

> @@ -1457,7 +1448,7 @@ static inline int lock_and_freeze_slab(s
> struct page *page)
> {
> if (slab_trylock(page)) {
> - __remove_partial(n, page);
> + remove_partial(n, page);
> return 1;
> }
> return 0;
> @@ -1574,12 +1565,17 @@ static void unfreeze_slab(struct kmem_ca
> if (page->inuse) {
>
> if (page->freelist) {
> + spin_lock(&n->list_lock);
> add_partial(n, page, tail);
> + spin_unlock(&n->list_lock);
> stat(s, tail ? DEACTIVATE_TO_TAIL : DEACTIVATE_TO_HEAD);
> } else {
> stat(s, DEACTIVATE_FULL);
> - if (kmem_cache_debug(s) && (s->flags & SLAB_STORE_USER))
> - add_full(n, page);
> + if (kmem_cache_debug(s) && (s->flags & SLAB_STORE_USER)) {
> + spin_lock(&n->list_lock);
> + add_full(s, n, page);
> + spin_unlock(&n->list_lock);
> + }
> }
> slab_unlock(page);
> } else {
> @@ -1595,7 +1591,9 @@ static void unfreeze_slab(struct kmem_ca
> * kmem_cache_shrink can reclaim any empty slabs from
> * the partial list.
> */
> + spin_lock(&n->list_lock);
> add_partial(n, page, 1);
> + spin_unlock(&n->list_lock);
> slab_unlock(page);
> } else {
> slab_unlock(page);
> @@ -2097,7 +2095,11 @@ static void __slab_free(struct kmem_cach
> * then add it.
> */
> if (unlikely(!prior)) {
> + struct kmem_cache_node *n = get_node(s, page_to_nid(page));
> +
> + spin_lock(&n->list_lock);
> add_partial(get_node(s, page_to_nid(page)), page, 1);
> + spin_unlock(&n->list_lock);
> stat(s, FREE_ADD_PARTIAL);
> }
>

Second call to get_node() is unnecessary, you can pass n.

> @@ -2111,7 +2113,11 @@ slab_empty:
> /*
> * Slab still on the partial list.
> */
> - remove_partial(s, page);
> + struct kmem_cache_node *n = get_node(s, page_to_nid(page));
> +
> + spin_lock(&n->list_lock);
> + remove_partial(n, page);
> + spin_unlock(&n->list_lock);
> stat(s, FREE_REMOVE_PARTIAL);
> }
> slab_unlock(page);
> @@ -2393,7 +2399,6 @@ static void early_kmem_cache_node_alloc(
> {
> struct page *page;
> struct kmem_cache_node *n;
> - unsigned long flags;
>
> BUG_ON(kmem_cache_node->size < sizeof(struct kmem_cache_node));
>
> @@ -2420,14 +2425,7 @@ static void early_kmem_cache_node_alloc(
> init_kmem_cache_node(n, kmem_cache_node);
> inc_slabs_node(kmem_cache_node, node, page->objects);
>
> - /*
> - * lockdep requires consistent irq usage for each lock
> - * so even though there cannot be a race this early in
> - * the boot sequence, we still disable irqs.
> - */
> - local_irq_save(flags);
> add_partial(n, page, 0);
> - local_irq_restore(flags);
> }
>
> static void free_kmem_cache_nodes(struct kmem_cache *s)

This is no longer true? It wouldn't have been changed with this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/