Re: [PATCH V2 1/2] slub: fix kmem cgroup bug in kmem_cache_alloc_bulk

[Vladimir Davydov <vdavydov@virtuozzo.com>]

On Thu, Nov 05, 2015 at 04:37:51PM +0100, Jesper Dangaard Brouer wrote:
...
> @@ -1298,7 +1298,6 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s,
>  	flags &= gfp_allowed_mask;
>  	kmemcheck_slab_alloc(s, flags, object, slab_ksize(s));
>  	kmemleak_alloc_recursive(object, s->object_size, 1, s->flags, flags);
> -	memcg_kmem_put_cache(s);
>  	kasan_slab_alloc(s, object);
>  }
>  
> @@ -2557,6 +2556,7 @@ redo:
>  		memset(object, 0, s->object_size);
>  
>  	slab_post_alloc_hook(s, gfpflags, object);
> +	memcg_kmem_put_cache(s);

Asymmetric - not good IMO. What about passing array of allocated objects
to slab_post_alloc_hook? Then we could leave memcg_kmem_put_cache where
it is now. I.e here we'd have

	slab_post_alloc_hook(s, gfpflags, &object, 1);

while in kmem_cache_alloc_bulk it'd look like

	slab_post_alloc_hook(s, flags, p, size);

right before return.

>  
>  	return object;
>  }
> @@ -2906,6 +2906,11 @@ bool kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  	struct kmem_cache_cpu *c;
>  	int i;
>  
> +	/* memcg and kmem_cache debug support */
> +	s = slab_pre_alloc_hook(s, flags);
> +	if (unlikely(!s))
> +		return false;
> +
>  	/*
>  	 * Drain objects in the per cpu slab, while disabling local
>  	 * IRQs, which protects against PREEMPT and interrupts
> @@ -2931,11 +2936,6 @@ bool kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  			continue; /* goto for-loop */
>  		}
>  
> -		/* kmem_cache debug support */
> -		s = slab_pre_alloc_hook(s, flags);
> -		if (unlikely(!s))
> -			goto error;
> -
>  		c->freelist = get_freepointer(s, object);
>  		p[i] = object;
>  
> @@ -2953,9 +2953,11 @@ bool kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  			memset(p[j], 0, s->object_size);
>  	}
>  
> +	memcg_kmem_put_cache(s);
>  	return true;
>  
>  error:
> +	memcg_kmem_put_cache(s);

It drops a reference to the cache so better to call it after you are
done with the cache, i.e. right before 'return false'.

Thanks,
Vladimir

>  	__kmem_cache_free_bulk(s, i, p);
>  	local_irq_enable();
>  	return false;
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>