On Wed, Jun 20, 2012 at 1:53 AM, Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> On Tue, Jun 19, 2012 at 08:45:03PM -0700, Ying Han wrote:
>> 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:
>> >> +{
>> >> + 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?
>
> Well, not in this patch as it stands. But once you squash the '0 per
> default', it may be good to note that this is a shortcut.
Will include some notes next time.
>
>> >> + 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.
>
> Only if root_mem_cgroup->use_hierarchy is set. At the same time, we
> usually behave as if this was the case, in accounting and reclaim.
>
> Right now we allow setting the soft limit in root_mem_cgroup but it
> does not make any sense. After your patch, even less so, because of
> these shortcut checks that now actually change semantics. Could we
> make this more consistent to users and forbid setting as soft limit in
> root_mem_cgroup? Patch below.
>
> The reason this behaves differently from hard limits is because the
> soft limits now have double meaning; they are upper limit and minimum
> guarantee at the same time. The unchangeable defaults in the root
> cgroup should be "no guarantee" and "unlimited soft limit" at the same
> time, but that is obviously not possible if these are opposing range
> ends of the same knob. So we pick no guarantees, always up for
> reclaim when looking top down but also behave as if the soft limit was
> unlimited in the root cgroup when looking bottom up.
>
> This is what the second check does. But I think it needs a clearer
> comment.
>
> ---
> From: Johannes Weiner <hannes@xxxxxxxxxxx>
> Subject: mm: memcg: forbid setting soft limit on root cgroup
>
> Setting a soft limit in the root cgroup does not make sense, as soft
> limits are enforced hierarchically and the root cgroup is the
> hierarchical parent of every other cgroup. It would not provide the
> discrimination between groups that soft limits are usually used for.
>
> With the current implementation of soft limits, it would only make
> global reclaim more aggressive compared to target reclaim, but we
> absolutely don't want anyone to rely on this behaviour.
>
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> ---
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ac35bcc..21c45a0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3905,6 +3967,10 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
> ret = mem_cgroup_resize_memsw_limit(memcg, val);
> break;
> case RES_SOFT_LIMIT:
> + if (mem_cgroup_is_root(memcg)) { /* Can't set limit on root */
> + ret = -EINVAL;
> + break;
> + }
> ret = res_counter_memparse_write_strategy(buffer, &val);
> if (ret)
> break;
Thanks, the patch makes sense to me and I will include in the next post.
--Ying
--
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