On Fri, May 02, 2014 at 04:15:15PM +0200, Michal Hocko wrote: > 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?
Thanks, that's much better IMO. > @@ -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 && respect_? consider_? force sounds like something the second round would do -- force reclaim despite guarantees... But then again, I'm still for removing that 2nd force cycle, so I don't care too strongly about that name (yet) :-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/