Re: [patch 2/2] fs: buffer: move allocation failure loop into the allocator

Andrew Morton
2013-12-04T02:06:25Z

On Wed, 4 Dec 2013 10:52:18 +0900 Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:

> SLUB already try to allocate high order page with clearing __GFP_NOFAIL.
> But, when allocating shadow page for kmemcheck, it missed clearing
> the flag. This trigger WARN_ON_ONCE() reported by Christian Casteyde.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=65991
>
> This patch fix this situation by using same allocation flag as original
> allocation.
>
> Reported-by: Christian Casteyde <casteyde.christian@free.fr>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 545a170..3dd28b1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1335,11 +1335,12 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
> page = alloc_slab_page(alloc_gfp, node, oo);
> if (unlikely(!page)) {
> oo = s->min;

What is the value of s->min? Please tell me it's zero.

> + alloc_gfp = flags;
> /*
> * Allocation may have failed due to fragmentation.
> * Try a lower order alloc if possible
> */
> - page = alloc_slab_page(flags, node, oo);
> + page = alloc_slab_page(alloc_gfp, node, oo);
>
> if (page)
> stat(s, ORDER_FALLBACK);

This change doesn't actually do anything.

> @@ -1349,7 +1350,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
> && !(s->flags & (SLAB_NOTRACK | DEBUG_DEFAULT_FLAGS))) {
> int pages = 1 << oo_order(oo);
>
> - kmemcheck_alloc_shadow(page, oo_order(oo), flags, node);
> + kmemcheck_alloc_shadow(page, oo_order(oo), alloc_gfp, node);

That seems reasonable, assuming kmemcheck can handle the allocation
failure.

Still I dislike this practice of using unnecessarily large allocations.
What does it gain us? Slightly improved object packing density.
Anything else?