Re: [PATCH 2/2] print vmalloc() state after allocation failures

[Dave Hansen]

On Sat, 2011-04-16 at 17:03 -0700, David Rientjes wrote:
> >  fail:
> > +     warn_alloc_failed(gfp_mask, order, "vmalloc: allocation failure, "
> > +                       "allocated %ld of %ld bytes\n",
> > +                       (area->nr_pages*PAGE_SIZE), area->size);
> >       vfree(area->addr);
> >       return NULL;
> >  }
> 
> Sorry, I still don't understand why this isn't just a three-liner patch to 
> call warn_alloc_failed().  I don't see the benefit of the "order" or 
> "tmp_mask" variables at all, they'll just be removed next time someone 
> goes down the mm/* directory and looks for variables that are used only 
> once or are unchanged as a cleanup. 

Without the "order" variable, we have:

	warn_alloc_failed(gfp_mask, 0, "vmalloc: allocation failure, "
		"allocated %ld of %ld bytes\n",
		(area->nr_pages*PAGE_SIZE), area->size);

I *HATE* those with a passion.  What is the '0' _doing_?  Is it for "0
pages", "do not print", "_do_ print"?  There's no way to tell without
going and finding warn_alloc_failed()'s definition.

With 'order' in there, the code self-documents, at least from the
caller's side.  It makes it 100% clear that the "0" being passed to the
allocators is that same as the one passed to the warning; it draws a
link between the allocations and the allocation error message:

	warn_alloc_failed(gfp_mask, order, "vmalloc: allocation failure, "
		"allocated %ld of %ld bytes\n",
		(area->nr_pages*PAGE_SIZE), area->size);

As for the 'tmp_mask' business.  Right now we have:

        for (i = 0; i < area->nr_pages; i++) {
                struct page *page;
+               gfp_t tmp_mask = gfp_mask | __GFP_NOWARN;

                if (node < 0)
-                       page = alloc_page(gfp_mask);
+                       page = alloc_page(tmp_mask);
                else
-                       page = alloc_pages_node(node, gfp_mask, 0);
+                       page = alloc_pages_node(node, tmp_mask, order);

The alternative is this:

        for (i = 0; i < area->nr_pages; i++) {
                struct page *page;

                if (node < 0)
-                       page = alloc_page(gfp_mask);
+                       page = alloc_page(gfp_mask | __GFP_NOWARN);
                else
-                       page = alloc_pages_node(node, gfp_mask, 0);
+                       page = alloc_pages_node(node, gfp_mask | __GFP_NOWARN,
+						order);

I can go look, but I bet the compiler compiles down to the same thing.
Plus, they're the same number of lines in the end.  I know which one
appeals to me visually.

I think we're pretty deep in personal preference territory here.  If I
hear a consensus that folks like it one way over another, I'm happy to
change it.

-- Dave

--
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/