On Mon, Nov 14, 2011 at 07:38:12PM +0100, Andrea Arcangeli wrote: > On Mon, Nov 14, 2011 at 02:04:21PM +0000, Mel Gorman wrote: > > In his fix, he avoided retrying the allocation if reclaim made no > > progress and __GFP_FS was not set. The problem is that this would > > result in GFP_NOIO allocations failing that previously succeeded > > which would be very unfortunate. > > GFP_NOFS are made by filesystems/buffers to avoid locking up on fs/vfs > locking. Those also should be able to handle failure gracefully but > userland is more likely to get a -ENOMEM from these (for example > during direct-io) if those fs allocs fails. I was also vaguely recalling Roland's talk at day 2 of kernel summit (reported at http://lwn.net/Articles/464500/) where he talked about error handling. One point he made was that some filesystems ran into problems in the event of memory allocation failure. I didn't audit if the block layer handles it better but one way or the other I did not want to throw the the block or filesystem layers curve balls. > So clearly it sounds risky > to apply the modification quoted above and risk having any GFP_NOFS > fail. Said that I'm afraid we're not deadlock safe with current code > that cannot fail but there's no easy solution and no way to fix it in > the short term, and it's only a theoretical concern. It's still a valid concern. The expectation is that we are protected from deadlocks a combination of mempools and the watermarks forcing processes to stall in direct reclaim leaving a cushion of pages for reclaim using PF_MEMALLOC to always make forward progress. Patches that break how watermarks work tend to lead to deadlock. > For !__GFP_FS allocations, __GFP_NOFAIL is the default for order <= > PAGE_ALLOC_COSTLY_ORDER and __GFP_NORETRY is the default for order > > PAGE_ALLOC_COSTLY_ORDER. This inconsistency is not so clean in my > view. Is your concern that the behaviour of the allocator changes quite significantly for orders < PAGE_ALLOC_COSTLY_ORDER? I agree with you that it would be nicer if there was a gradual scaling back of how much work the allocator did that depended on order. To date there has not been much pressure or motivation to implement it. > Also for GFP_KERNEL/USER/__GFP_FS regular allocations the > __GFP_NOFAIL looks more like a __GFP_MAY_OOM. But if we fix that and > we drop __GFP_NORETRY, and we set __GFP_NOFAIL within the > GFP_NOFS/NOIO #defines (to remove the magic PAGE_ALLOC_COSTLY_ORDER > check in should_alloc_retry) we may loop forever if somebody allocates > several mbytes of huge contiguous RAM with GFP_NOIO. So at least > there's a practical explanation for the current code. > Yep. > Patch looks good to me (and safer) even if I don't like keeping > infinite loops from a purely theoretical standpoint. >From a more practical point of view, I am generally more concerned with abnormally large stalls from within the page allocator which is what patches like "Do not stall in synchronous compaction for THP allocations" address. -- Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>