Re: [PATCH 2/2] memcg: Allow hard guarantee mode for low limit reclaim

Tejun Heo
2014-06-06T15:29:20Z

Hello, Michal.

On Fri, Jun 06, 2014 at 04:46:50PM +0200, Michal Hocko wrote:
> +choice
> + prompt "Memory Resource Controller reclaim protection"
> + depends on MEMCG
> + help

Why is this necessary?

- This doesn't affect boot.

- memcg requires runtime config *anyway*.

- The config is inherited from the parent, so the default flipping
isn't exactly difficult.

Please drop the kconfig option.

> +static int mem_cgroup_write_reclaim_strategy(struct cgroup_subsys_state *css, struct cftype *cft,
> + char *buffer)
> +{
> + struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> + int ret = 0;
> +
> + if (!strncmp(buffer, "low_limit_guarantee",
> + sizeof("low_limit_guarantee"))) {
> + memcg->hard_low_limit = true;
> + } else if (!strncmp(buffer, "low_limit_best_effort",
> + sizeof("low_limit_best_effort"))) {
> + memcg->hard_low_limit = false;
> + } else
> + ret = -EINVAL;
> +
> + return ret;
> +}

So, ummm, this raises a big red flag for me. You're now implementing
two behaviors in a mostly symmetric manner to soft/hard limits but
choosing a completely different scheme in how they're configured
without any rationale.

* Are you sure soft and hard guarantees aren't useful when used in
combination? If so, why would that be the case?

* We have pressure monitoring interface which can be used for soft
limit pressure monitoring. How should breaching soft guarantee be
factored into that? There doesn't seem to be any way of notifying
that at the moment? Wouldn't we want that to be integrated into the
same mechanism?

What scares me the most is that you don't even seem to have noticed
the asymmetry and are proposing userland-facing interface without
actually thinking things through. This is exactly how we've been
getting into trouble.

For now, for everything.

Nacked-by: Tejun Heo <tj@kernel.org>

Thanks.

--
tejun