On Fri 02-05-14 09:01:18, Johannes Weiner wrote: > On Fri, May 02, 2014 at 02:07:15PM +0200, Michal Hocko wrote: > > On Fri 02-05-14 11:36:28, Michal Hocko wrote: > > > On Wed 30-04-14 18:55:50, Johannes Weiner wrote: > > > > On Mon, Apr 28, 2014 at 02:26:42PM +0200, Michal Hocko wrote: > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > > index 19d620b3d69c..40e517630138 100644 > > > > > --- a/mm/memcontrol.c > > > > > +++ b/mm/memcontrol.c > > > > > @@ -2808,6 +2808,29 @@ static struct mem_cgroup > > > > > *mem_cgroup_lookup(unsigned short id) > > > > > return mem_cgroup_from_id(id); > > > > > } > > > > > > > > > > +/** > > > > > + * mem_cgroup_reclaim_eligible - checks whether given memcg is > > > > > eligible for the > > > > > + * reclaim > > > > > + * @memcg: target memcg for the reclaim > > > > > + * @root: root of the reclaim hierarchy (null for the global reclaim) > > > > > + * > > > > > + * The given group is reclaimable if it is above its low limit and > > > > > the same > > > > > + * applies for all parents up the hierarchy until root (including). > > > > > + */ > > > > > +bool mem_cgroup_reclaim_eligible(struct mem_cgroup *memcg, > > > > > + struct mem_cgroup *root) > > > > > > > > Could you please rename this to something that is more descriptive in > > > > the reclaim callsite? How about mem_cgroup_within_low_limit()? > > > > > > I have intentionally used somethig that is not low_limit specific. The > > > generic reclaim code does't have to care about the reason why a memcg is > > > not reclaimable. I agree that having follow_low_limit paramter explicit > > > and mem_cgroup_reclaim_eligible not is messy. So something should be > > > renamed. I would probably go with > > > s@follow_low_limit@check_reclaim_eligible@ > > > but I do not have a strong preference. > > > > What about this? > > I really don't like it. > > Yes, we should be hiding implementation details, but we should stop > treating memcg like an alien in this code. The VM code obviously > doesn't have to know HOW the guarantees are exactly implemented, but > it's a perfectly fine *concept* that can be known outside of memcg: > > shrink_zone: > for each memcg in system: > if mem_cgroup_within_guarantee(memcg): > continue > reclaim(memcg-zone) > > is perfectly understandable and makes it easier to reason about the > behavior of the reclaim code. If I just see !mem_cgroup_eligible(), I > don't know if this affects the scenario I'm thinking about at all. > > It's obscuring useful information for absolutely no benefit. If you > burden the reclaim code with a callback, you better explain what you > are doing. You owe it to the reader.
OK fair enough, what about the following? --- >From 4e0404fa2888d04de80f33fcb76712b0fbd44e1c Mon Sep 17 00:00:00 2001 From: Michal Hocko <[email protected]> Date: Fri, 2 May 2014 16:12:41 +0200 Subject: [PATCH] mmotm: memcg-mm-introduce-lowlimit-reclaim-fix.patch mem_cgroup_reclaim_eligible -> mem_cgroup_within_guarantee as suggested by Johannes. --- include/linux/memcontrol.h | 6 +++--- mm/memcontrol.c | 15 ++++++++------- mm/vmscan.c | 25 ++++++++++++++++--------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 6c59056f4bc6..c00ccc5f70b9 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -92,7 +92,7 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg, bool task_in_mem_cgroup(struct task_struct *task, const struct mem_cgroup *memcg); -extern bool mem_cgroup_reclaim_eligible(struct mem_cgroup *memcg, +extern bool mem_cgroup_within_guarantee(struct mem_cgroup *memcg, struct mem_cgroup *root); extern struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page); @@ -291,10 +291,10 @@ static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page, return &zone->lruvec; } -static inline bool mem_cgroup_reclaim_eligible(struct mem_cgroup *memcg, +static inline bool mem_cgroup_within_guarantee(struct mem_cgroup *memcg, struct mem_cgroup *root) { - return true; + return false; } static inline struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 7a276c0d141e..58982d18f6ea 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2810,26 +2810,27 @@ static struct mem_cgroup *mem_cgroup_lookup(unsigned short id) } /** - * mem_cgroup_reclaim_eligible - checks whether given memcg is eligible for the - * reclaim + * mem_cgroup_within_guarantee - checks whether given memcg is within its + * memory guarantee * @memcg: target memcg for the reclaim * @root: root of the reclaim hierarchy (null for the global reclaim) * - * The given group is reclaimable if it is above its low limit and the same - * applies for all parents up the hierarchy until root (including). + * The given group is within its reclaim gurantee if it is below its low limit + * or the same applies for any parent up the hierarchy until root (including). + * Such a group might be excluded from the reclaim. */ -bool mem_cgroup_reclaim_eligible(struct mem_cgroup *memcg, +bool mem_cgroup_within_guarantee(struct mem_cgroup *memcg, struct mem_cgroup *root) { do { if (!res_counter_low_limit_excess(&memcg->res)) - return false; + return true; if (memcg == root) break; } while ((memcg = parent_mem_cgroup(memcg))); - return true; + return false; } struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page) diff --git a/mm/vmscan.c b/mm/vmscan.c index 0f428158254e..20ca95fbaebb 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2215,8 +2215,18 @@ static inline bool should_continue_reclaim(struct zone *zone, } } +/** + * __shrink_zone - shrinks a given zone + * + * @zone: zone to shrink + * @sc: scan control with additional reclaim parameters + * @force_memcg_guarantee: do not reclaim memcgs which are within their memory + * guarantee + * + * Returns the number of reclaimed memcgs. + */ static unsigned __shrink_zone(struct zone *zone, struct scan_control *sc, - bool follow_low_limit) + bool force_memcg_guarantee) { unsigned long nr_reclaimed, nr_scanned; unsigned nr_scanned_groups = 0; @@ -2236,12 +2246,9 @@ static unsigned __shrink_zone(struct zone *zone, struct scan_control *sc, do { struct lruvec *lruvec; - /* - * Memcg might be under its low limit so we have to - * skip it during the first reclaim round - */ - if (follow_low_limit && - !mem_cgroup_reclaim_eligible(memcg, root)) { + /* Memcg might be protected from the reclaim */ + if (force_memcg_guarantee && + mem_cgroup_within_guarantee(memcg, root)) { /* * It would be more optimal to skip the memcg * subtree now but we do not have a memcg iter @@ -2289,8 +2296,8 @@ static void shrink_zone(struct zone *zone, struct scan_control *sc) if (!__shrink_zone(zone, sc, true)) { /* * First round of reclaim didn't find anything to reclaim - * because of low limit protection so try again and ignore - * the low limit this time. + * because of the memory guantees for all memcgs in the + * reclaim target so try again and ignore guarantees this time. */ __shrink_zone(zone, sc, false); } -- 2.0.0.rc0 -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
