On Tue, 19 Jun 2018, Michal Hocko wrote: > On Mon 18-06-18 18:11:26, Mikulas Patocka wrote: > [...] > > I grepped the kernel for __GFP_NORETRY and triaged them. I found 16 cases > > without a fallback - those are bugs that make various functions randomly > > return -ENOMEM. > > Well, maybe those are just optimistic attempts to allocate memory and > have a fallback somewhere else. So I would be careful calling them > outright bugs. But maybe you are right. I was trying to find the fallback code when I triaged them and maked as "BUG" those cases where I didn't find it. You can search harder and perhaps you'll find something that I didn't. > > Most of the callers provide callback. > > > > There is another strange flag - __GFP_RETRY_MAYFAIL - it provides two > > different functions - if the allocation is larger than > > PAGE_ALLOC_COSTLY_ORDER, it retries the allocation as if it were smaller. > > If the allocations is smaller than PAGE_ALLOC_COSTLY_ORDER, > > __GFP_RETRY_MAYFAIL will avoid the oom killer (larger order allocations > > don't trigger the oom killer at all). > > Well, the primary purpose of this flag is to provide a consistent > failure behavior for all requests regardless of the size. > > > So, perhaps __GFP_RETRY_MAYFAIL could be used instead of __GFP_NORETRY in > > the cases where the caller wants to avoid trigerring the oom killer (the > > problem is that __GFP_NORETRY causes random failure even in no-oom > > situations but __GFP_RETRY_MAYFAIL doesn't). > > myabe yes. > > > So my suggestion is - fix these obvious bugs when someone allocates memory > > with __GFP_NORETRY without any fallback - and then, __GFP_NORETRY could be > > just changed to return NULL instead of sleeping. > > No real objection to fixing wrong __GFP_NORETRY usage. But __GFP_NORETRY > can sleep. Nothing will really change in that regards. It does a > reclaim and that _might_ sleep. > > But seriously, isn't the best way around the throttling issue to use > PF_LESS_THROTTLE? Yes - it could be done by setting PF_LESS_THROTTLE. But I think it would be better to change it just in one place than to add PF_LESS_THROTTLE to every block device driver (because adding it to every block driver results in more code). What about this patch? If __GFP_NORETRY and __GFP_FS is not set (i.e. the request comes from a block device driver or a filesystem), we should not sleep. Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> Cc: stable@xxxxxxxxxxxxxxx Index: linux-2.6/mm/vmscan.c =================================================================== --- linux-2.6.orig/mm/vmscan.c +++ linux-2.6/mm/vmscan.c @@ -2674,6 +2674,7 @@ static bool shrink_node(pg_data_t *pgdat * the LRU too quickly. */ if (!sc->hibernation_mode && !current_is_kswapd() && + (sc->gfp_mask & (__GFP_NORETRY | __GFP_FS)) != __GFP_NORETRY && current_may_throttle() && pgdat_memcg_congested(pgdat, root)) wait_iff_congested(BLK_RW_ASYNC, HZ/10);
