Re: [PATCH V3 3/3] mm: page_alloc: drain pcp lists before oom kill

[Michal Hocko <mhocko@suse.com>]

On Mon 07-09-26 01:49:39, Yosry Ahmed wrote:
> On Mon, Sep 7, 2026 at 12:26 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Fri 04-09-26 09:35:55, Yosry Ahmed wrote:
> > > On Fri, Sep 4, 2026 at 9:27 AM Michal Hocko <mhocko@suse.com> wrote:
> > [...]
> > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > index 8d79f76cdd0e1..98e9079240ad5 100644
> > > > > --- a/mm/page_alloc.c
> > > > > +++ b/mm/page_alloc.c
> > > > > @@ -4592,11 +4592,12 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask,
> > > > > unsigned int order,
> > > > >         psi_memstall_enter(&pflags);
> > > > >         *did_some_progress = __perform_reclaim(gfp_mask, order, ac);
> > > > >         if (unlikely(!(*did_some_progress)))
> > > > > -               goto out;
> > > > > +               goto drain;
> > > > >
> > > > >  retry:
> > > > >         page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac);
> > > > >
> > > > > +drain:
> > > > >         /*
> > > > >          * If an allocation failed after direct reclaim, it could be because
> > > > >          * pages are pinned on the per-cpu lists or in high alloc reserves.
> > > > > @@ -4608,7 +4609,6 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask,
> > > > > unsigned int order,
> > > > >                 drained = true;
> > > > >                 goto retry;
> > > > >         }
> > > > > -out:
> > > > >         psi_memstall_leave(&pflags);
> > > >
> > > > Ideally if we can make the function call less hairy. Maybe we want to
> > > > make draining part of the reclaim as the last resort when normal reclaim
> > > > fails.
> > >
> > > Do you mean do the draining in __perform_reclaim(), or deeper into the
> > > reclaim stack?
> > >
> > > The thing is that __alloc_pages_direct_reclaim() currently drains when
> > > __perform_reclaim() fails to make any progress and we still cannot
> > > allocate. The change above makes it drain if it cannot allocate after
> > > __perform_reclaim(), regardless of progress. So if you want to move it
> > > into __perform_reclaim(), we'll have it in both places.
> > >
> > > Or maybe I just don't understand what you meant :)
> >
> > Sorry for not being clear enough. I meant to pull draining out of
> > __alloc_pages_direct_reclaim and instead have it somewhere in the
> > reclaim path. It is not entirely clear to me  where at the moment but we
> > do not need to have the same behavior as now. The idea behind the code
> > is to not drain way too much. Maybe we want to drain when dropping the
> > priority down to 0.
> 
> If we want to maintain the current behavior of only doing this for
> direct reclaim (not kswapd, cgroup reclaim, or proactive reclaim),
> then I was going to suggest adding it to do_try_to_free_pages().
> However, we bail before priority reaches 0 if we are able to make
> progress or compaction is ready.
> 
> Also, I think in do_try_to_free_pages() we don't have enough context
> to decide if we need to drain the pcplists. Looking at the comment in
> __alloc_pages_direct_reclaim(), we specifically drain the pcplists if
> the allocation fails after reclaim makes progress to make sure all
> reclaimed pages (e.g. in other CPUs' pcplists) are made available to
> the allocation. So I think it fits better in the allocation path, so
> that we only drain if we cannot allocate after direct reclaim.
> 
> I think the main issue is that we only drain today if we know direct
> reclaim made progress, so it potentially freed some pages to pcplists.
> However, it is possible that reclaim did not make progress but there
> were already pages on the pcplists (e.g. freed concurrently or were
> already there). I don't think the right place to do this is reclaim
> path.
> 
> I personally think either Charan's original patch (draining in
> should_reclaim_retry()), or the diff I proposed upthread (draining in
> __alloc_pages_direct_reclaim()) are probably the best places. Please
> let me know if you still disagree.

In that case should_reclaim_retry seems a better fitting fix. 
Thanks!
-- 
Michal Hocko
SUSE Labs