Re: __GFP_NOFAIL and oom_killer_disabled?

Michal Hocko
2015-02-25T16:02:28Z

On Wed 25-02-15 20:22:22, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > This commit hasn't introduced any behavior changes. GFP_NOFAIL
> > allocations fail when OOM killer is disabled since beginning
> > 7f33d49a2ed5 (mm, PM/Freezer: Disable OOM killer when tasks are frozen).
>
> I thought that
>
> - out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false);
> - *did_some_progress = 1;
> + if (out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false))
> + *did_some_progress = 1;
>
> in commit c32b3cbe0d067a9c "oom, PM: make OOM detection in the freezer
> path raceless" introduced a code path which fails to set
> *did_some_progress to non 0 value.

But this commit had also the following hunk:
@@ -2317,9 +2315,6 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,

*did_some_progress = 0;

- if (oom_killer_disabled)
- return NULL;
-

so we even wouldn't get down to out_of_memory and returned with
did_some_progress=0 right away. So the patch hasn't changed the logic.

> > "
> > We haven't seen any bug reports since 2009 so I haven't marked the patch
> > for stable. I have no problem to backport it to stable trees though if
> > people think it is a good precaution.
> > "
>
> Until 3.18, GFP_NOFAIL for GFP_NOFS / GFP_NOIO did not fail with
> oom_killer_disabled == true because of
>
> ----------
> if (!did_some_progress) {
> if (oom_gfp_allowed(gfp_mask)) {
> if (oom_killer_disabled)
> goto nopage;
> (...snipped...)
> goto restart;
> }
> }
> (...snipped...)
> goto rebalance;
> ----------
>
> and that might be the reason you did not see bug reports.
> In 3.19, GFP_NOFAIL for GFP_NOFS / GFP_NOIO started to fail with
> oom_killer_disabled == true because of

OK, that would change the bahavior for __GFP_NOFAIL|~__GFP_FS
allocations. The patch from Johannes which reverts GFP_NOFS failure mode
should go to stable and that should be sufficient IMO.

[...]

> So, it is commit 9879de7373fc "mm: page_alloc: embed OOM killing naturally
> into allocation slowpath" than commit c32b3cbe0d067a9c "oom, PM: make OOM
> detection in the freezer path raceless" that introduced behavior changes?

Yes.

--
Michal Hocko
SUSE Labs