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

[Tejun Heo]

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@xxxxxxxxxx>

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/