Re: [PATCH V8 1/2] mm: memcg softlimit reclaim rework

[Michal Hocko <mhocko@suse.cz>]

On Fri 03-08-12 09:34:11, Ying Han wrote:
> On Fri, Aug 3, 2012 at 9:16 AM, Rik van Riel <riel@redhat.com> wrote:
> > On 08/03/2012 11:22 AM, Michal Hocko wrote:
> >>
> >> On Thu 02-08-12 14:24:18, Ying Han wrote:
> >> [...]
> >>>
> >>> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >>> index 3e0d0cd..88487b3 100644
> >>> --- a/mm/vmscan.c
> >>> +++ b/mm/vmscan.c
> >>> @@ -1866,7 +1866,22 @@ static void shrink_zone(struct zone *zone, struct
> >>> scan_control *sc)
> >>>         do {
> >>>                 struct lruvec *lruvec = mem_cgroup_zone_lruvec(zone,
> >>> memcg);
> >>>
> >>> -               shrink_lruvec(lruvec, sc);
> >>> +               /*
> >>> +                * Reclaim from mem_cgroup if any of these conditions are
> >>> met:
> >>> +                * - this is a targetted reclaim ( not global reclaim)
> >>> +                * - reclaim priority is less than DEF_PRIORITY
> >>> +                * - mem_cgroup or its ancestor ( not including root
> >>> cgroup)
> >>> +                * exceeds its soft limit
> >>> +                *
> >>> +                * Note: The priority check is a balance of how hard to
> >>> +                * preserve the pages under softlimit. If the memcgs of
> >>> the
> >>> +                * zone having trouble to reclaim pages above their
> >>> softlimit,
> >>> +                * we have to reclaim under softlimit instead of burning
> >>> more
> >>> +                * cpu cycles.
> >>> +                */
> >>> +               if (!global_reclaim(sc) || sc->priority<  DEF_PRIORITY ||
> >>> +                               mem_cgroup_over_soft_limit(memcg))
> >>> +                       shrink_lruvec(lruvec, sc);
> >>>
> >>>                 /*
> >>>                  * Limit reclaim has historically picked one memcg and
> >>
> >>
> >> I am thinking that we could add a constant for the priority
> >> limit. Something like
> >> #define MEMCG_LOW_SOFTLIMIT_PRIORITY    DEF_PRIORITY
> >>
> >> Although it doesn't seem necessary at the moment, because there is just
> >> one location where it matters but it could help in the future.
> >> What do you think?
> >
> >
> > I am working on changing the code to find the "highest priority"
> > LRU and reclaim from that list first.  That will obviate the need
> > for such a change. However, the other cleanups and simplifications
> > made by Ying's patch are good to have...
> 
> So what you guys think to take from here. I can make the change as
> Michal suggested if that would be something helpful future changes.
> However, I wonder whether or not it is necessary.

I am afraid we will not move forward without a proper implementation of
the "nobody under soft limit" case. Maybe Rik's idea would just work out
but this patch on it's own could regress so taking it separately is no
go IMO. I like how it reduces the code size but we are not "there" yet...

> 
> --Ying
> 
> >
> > --
> > All rights reversed

-- 
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>