Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Andrew Morton]

On Sat, 9 May 2009 11:19:11 +0200 Ingo Molnar <mingo@xxxxxxx> wrote:

> 
> * Pekka Enberg <penberg@xxxxxxxxxxxxxx> wrote:
> 
> > Hi Andrew,
> >
> > Andrew Morton wrote:
> >> On Fri, 08 May 2009 18:10:28 +0300
> >> Pekka Enberg <penberg@xxxxxxxxxxxxxx> wrote:
> >>
> >>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
> >>
> >> urgh, you have to be kidding me.  This significantly worsens complexity
> >> and risk in core MM and it's just yuk.
> >>
> >> I think we can justify pulling such dopey party tricks to save 
> >> pageframe space, or bits in page.flags and such.  But just to 
> >> save a scrap of memory which would have been released during boot 
> >> anwyay?  Don't think so.
> >
> > No, I wasn't kidding and I don't agree that it "significantly 
> > worsens complexity". The point is not to save memory but to 
> > clearly annotate those special call-sites that really don't need 
> > to check for out-of-memory.
> 
> Frankly, i cannot believe that so many smart people dont see the 
> simple, universal, un-arguable truism in the following statement:
> 
>  it is shorter, tidier, more maintainable, more reviewable to write:
> 
> 	ptr = kmalloc(GFP_BOOT, size);
> 
>  than to write:
> 
> 	ptr = kmalloc(GFP_KERNEL, size);
> 	BUG_ON(!ptr);
> 
> We have a lot of such patterns in platform code. Dozens and dozens 
> of them.
> 
> There _might_ be some more nuanced second-level arguments: "yes, I 
> agree in principle, but complexity elsewhere or other side-effects 
> make this a net negative change."
> 
> Alas, those arguments would be wrong too:
> 
>  - we have a lot of such patterns and GFP_BOOT is unambigious 
> 
>  - post-bootup mis-use of GFP_BOOT could be warned about 
>    unconditionally if used after free_initmem(), if we care enough.
> 
>  - Pekka's patch is dead simple. There's no "complexity" anywhere.
> 
> Agreeing to this and introducing this should have been a matter of 
> 30 seconds of thinking. Why are we still arguing about this? Dont we 
> have enough bugs to worry about?
> 

Your reply has nothing at all to do with the email to which you
replied.  How strange.


Here's an example:

void *some_library_function(..., gfp_t gfpflags)
{
	...
	p = kmalloc(..., gfpflags | __GFP_NOFAIL);
	...
}

Now here we have a nice little hand-grenade.  If someone accidentally
does

	some_library_function(..., GFP_KERNEL | __GFP_NORETRY);

their code will happily pass testing.  Except one day someone's kernel
will run out of memory and instead of handling it properly, their kernel
will panic.

Another issue is that now all memory allocation functions which inspect
__GFP_NOFAIL or __GFP_NORETRY need to remember to check for the other
flag to filter out GFP_PANIC.  


Those two flags were well-chosen and it's a good bet that they will
never be used together.  But it's not certain, and the _results_ of
that mistake are really bad: a very small number of machines will crash
very rarely.


We have plenty of bits free there - six, I think.  And we could get
four more if needed by overlaying the `enum mapping_flags' on top of
certain __GFP_* flags and adding appropriate masking in
mapping_gfp_mask().

So I suggest that we add a new __GFP_foo.

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