On Tue, Jun 19, 2012 at 4:29 AM, Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> On Mon, Jun 18, 2012 at 09:47:27AM -0700, Ying Han wrote:
>> This patch reverts all the existing softlimit reclaim implementations and
>> instead integrates the softlimit reclaim into existing global reclaim logic.
>>
>> The new softlimit reclaim includes the following changes:
>>
>> 1. add function should_reclaim_mem_cgroup()
>>
>> Add the filter function should_reclaim_mem_cgroup() under the common function
>> shrink_zone(). The later one is being called both from per-memcg reclaim as
>> well as global reclaim.
>>
>> Today the softlimit takes effect only under global memory pressure. The memcgs
>> get free run above their softlimit until there is a global memory contention.
>> This patch doesn't change the semantics.
>
> But it's quite a performance regression. Maybe it would be better
> after all to combine this change with 'make 0 the default'?
>
> Yes, I was the one asking for the changes to be separated, if
> possible, but I didn't mean regressing in between. No forward
> dependencies in patch series, please.
Ok, I don't have problem to squash that patch in next time.
>
>> Under the global reclaim, we try to skip reclaiming from a memcg under its
>> softlimit. To prevent reclaim from trying too hard on hitting memcgs
>> (above softlimit) w/ only hard-to-reclaim pages, the reclaim priority is used
>> to skip the softlimit check. This is a trade-off of system performance and
>> resource isolation.
>>
>> 2. "hierarchical" softlimit reclaim
>>
>> This is consistant to how softlimit was previously implemented, where the
>> pressure is put for the whole hiearchy as long as the "root" of the hierarchy
>> over its softlimit.
>>
>> This part is not in my previous posts, and is quite different from my
>> understanding of softlimit reclaim. After quite a lot of discussions with
>> Johannes and Michal, i decided to go with it for now. And this is designed
>> to work with both trusted setups and untrusted setups.
>
> This may be really confusing to someone uninvolved reading the
> changelog as it doesn't have anything to do with what the patch
> actually does.
>
> It may be better to include past discussion outcomes in the
> introductary email of a series.
I will try to include some of the points from our last discussion in
the commit log.
>> @@ -870,8 +672,6 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
>> preempt_enable();
>>
>> mem_cgroup_threshold(memcg);
>> - if (unlikely(do_softlimit))
>> - mem_cgroup_update_tree(memcg, page);
>> #if MAX_NUMNODES > 1
>> if (unlikely(do_numainfo))
>> atomic_inc(&memcg->numainfo_events);
>> @@ -922,6 +722,31 @@ struct mem_cgroup *try_get_mem_cgroup_from_mm(struct mm_struct *mm)
>> return memcg;
>> }
>>
>> +bool should_reclaim_mem_cgroup(struct mem_cgroup *memcg)
>
> I'm not too fond of the magical name. The API provides an information
> about soft limits, the decision should rest with vmscan.c.
>
> mem_cgroup_over_soft_limit() e.g.?
That is fine w/ me.
>
>> +{
>> + if (mem_cgroup_disabled())
>> + return true;
>> +
>> + /*
>> + * We treat the root cgroup special here to always reclaim pages.
>> + * Now root cgroup has its own lru, and the only chance to reclaim
>> + * pages from it is through global reclaim. note, root cgroup does
>> + * not trigger targeted reclaim.
>> + */
>> + if (mem_cgroup_is_root(memcg))
>> + return true;
>
> With the soft limit at 0, the comment is no longer accurate because
> this check turns into a simple optimization. We could check the
> res_counter soft limit, which would always result in the root group
> being above the limit, but we take the short cut.
For root group, my intention here is always reclaim pages from it
regardless of the softlimit setting. And the reason is exactly the one
in the comment. If the softlimit is set to 0 as default, I agree this
is then a short cut.
Anything you suggest that I need to change here?
>
>> + for (; memcg; memcg = parent_mem_cgroup(memcg)) {
>> + /* This is global reclaim, stop at root cgroup */
>> + if (mem_cgroup_is_root(memcg))
>> + break;
>
> I don't see why you add this check and the comment does not help.
The root cgroup would have softlimit set to 0 ( in most of the cases
), and not skipping root will make everyone reclaimable here.
Thank you for reviewing !
--Ying
>
>> + if (res_counter_soft_limit_excess(&memcg->res))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> /**
>> * mem_cgroup_iter - iterate over memory cgroup hierarchy
>> * @root: hierarchy root
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href