Re: [PATCH v3 RFC] mm/vmscan: more restrictive condition for retry of shrink_zones

[Michal Hocko <mhocko@kernel.org>]

On Mon 13-03-17 08:17:56, Shakeel Butt wrote:
> On Mon, Mar 13, 2017 at 1:33 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > Please do not post new version after a single feedback and try to wait
> > for more review to accumulate. This is in the 3rd version and it is not
> > clear why it is still an RFC.
> >
> > On Sun 12-03-17 19:06:10, Yisheng Xie wrote:
> >> From: Yisheng Xie <xieyisheng1@huawei.com>
> >>
> >> When we enter do_try_to_free_pages, the may_thrash is always clear, and
> >> it will retry shrink zones to tap cgroup's reserves memory by setting
> >> may_thrash when the former shrink_zones reclaim nothing.
> >>
> >> However, when memcg is disabled or on legacy hierarchy, it should not do
> >> this useless retry at all, for we do not have any cgroup's reserves
> >> memory to tap, and we have already done hard work but made no progress.
> >>
> >> To avoid this time costly and useless retrying, add a stub function
> >> mem_cgroup_thrashed() and return true when memcg is disabled or on
> >> legacy hierarchy.
> >
> > Have you actually seen this as a bad behavior? On which workload? Or
> > have spotted this by the code review?
> >
> > Please note that more than _what_ it is more interesting _why_ the patch
> > has been prepared.
> >
> > I agree the current additional round of reclaim is just lame because we
> > are trying hard to control the retry logic from the page allocator which
> > is a sufficient justification to fix this IMO. But I really hate the
> > name. At this point we do not have any idea that the memcg is trashing
> > as the name of the function suggests.
> >
> > All of them simply might not have any reclaimable pages. So I would
> > suggest either a better name e.g. memcg_allow_lowmem_reclaim() or,
> > preferably, fix this properly. E.g. something like the following.
> > ---
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index bae698484e8e..989ba9761921 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -99,6 +99,9 @@ struct scan_control {
> >         /* Can cgroups be reclaimed below their normal consumption range? */
> >         unsigned int may_thrash:1;
> >
> > +       /* Did we have any memcg protected by the low limit */
> > +       unsigned int memcg_low_protection:1;
> > +
> >         unsigned int hibernation_mode:1;
> >
> >         /* One of the zones is ready for compaction */
> > @@ -2513,6 +2516,7 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
> >                         if (mem_cgroup_low(root, memcg)) {
> >                                 if (!sc->may_thrash)
> >                                         continue;
> > +                               sc->memcg_low_protection = true;
> 
> I think you wanted to put this statement before the continue otherwise
> it will just disable the sc->may_thrash (second reclaim pass)
> altogether.

yes, of course, just a quick and dirty hack to show my point.
Sorry about the confusion.
 
> >                                 mem_cgroup_events(memcg, MEMCG_LOW, 1);
> >                         }
> >
> > @@ -2774,7 +2778,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
> >                 return 1;
> >
> >         /* Untapped cgroup reserves?  Don't OOM, retry. */
> > -       if (!sc->may_thrash) {
> > +       if ( sc->memcg_low_protection && !sc->may_thrash) {
> >                 sc->priority = initial_priority;
> >                 sc->may_thrash = 1;
> >                 goto retry;
> > --
> > Michal Hocko
> > SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs