Re: [PATCH v2 2/5] mm, memcg: introduce MEMCG_PROT_SKIP for memcg zero usage case

[Roman Gushchin <guro@fb.com>]

On Tue, Dec 24, 2019 at 02:53:23AM -0500, Yafang Shao wrote:
> If the usage of a memcg is zero, we don't need to do useless work to scan
> it. That is a minor optimization.

The optimization isn't really related to the main idea of the patchset,
so I'd prefer to treat it separately.

> 
> Cc: Roman Gushchin <guro@fb.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/memcontrol.h | 1 +
>  mm/memcontrol.c            | 2 +-
>  mm/vmscan.c                | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 612a457..1a315c7 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -54,6 +54,7 @@ enum mem_cgroup_protection {
>  	MEMCG_PROT_NONE,
>  	MEMCG_PROT_LOW,
>  	MEMCG_PROT_MIN,
> +	MEMCG_PROT_SKIP,	/* For zero usage case */
>  };
>  
>  struct mem_cgroup_reclaim_cookie {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c5b5f74..f35fcca 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6292,7 +6292,7 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root,
>  
>  	usage = page_counter_read(&memcg->memory);
>  	if (!usage)
> -		return MEMCG_PROT_NONE;
> +		return MEMCG_PROT_SKIP;

I'm concerned that it might lead to a regression with the scraping of
last pages from a memcg. Charge is batched using percpu stocks, so the
value of the page counter is approximate. Skipping the cgroup entirely
we're losing all chances to reclaim these few pages.

Idk how serious the problem could be in the real life, and maybe it's OK
to skip if the cgroup is online, but I'd triple check here.

Also, because this optimization isn't really related to protection,
why not check the page counter first, e.g.:

	memcg = mem_cgroup_iter(root, NULL, NULL);
 	do {
		unsigned long reclaimed;
		unsigned long scanned;

		if (!page_counter_read(&memcg->memory))
			continue;

		switch (mem_cgroup_protected(root, memcg)) {
		case MEMCG_PROT_MIN:
			/*
			 * Hard protection.
			 * If there is no reclaimable memory, OOM.
			 */
			continue;
		case MEMCG_PROT_LOW:

--

Thank you!