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

[Yosry Ahmed]

On Mon, Sep 7, 2026 at 12:26 AM Michal Hocko <mhocko@xxxxxxxx> wrote:
>
> On Fri 04-09-26 09:35:55, Yosry Ahmed wrote:
> > On Fri, Sep 4, 2026 at 9:27 AM Michal Hocko <mhocko@xxxxxxxx> 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.

>
> --
> Michal Hocko
> SUSE Labs