Re: [PATCH 1/9] memcg: make the v1 soft limit knob inert

On Tue 11-08-26 13:31:55, Shakeel Butt wrote:
> The v1 soft limit has been deprecated since v6.12 and nobody has
> reported depending on it. Start the removal by decoupling the interface
> from the implementation: keep memory.soft_limit_in_bytes, but ignore
> writes to it and always report the maximum value on read similar to
> what memory.kmem.limit_in_bytes already does.
> 
> Writes are still parsed, so malformed input keeps returning -EINVAL.
> The knob now also behaves the same everywhere: it used to return
> -EOPNOTSUPP on PREEMPT_RT, where soft limit reclaim has always been
> disabled.

Is there any specific reason to not return EOPNOTSUPP for everybody now?

> This also fixes the syzbot report linked below. Soft limit reclaim is
> the only caller that runs shrink_lruvec() from kswapd against a
> specific memcg, so it is the only way to reach lru_gen_shrink_lruvec()
> and in turn set_mm_walk(), which warns when called from kswapd.
> 
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Shakeel Butt <[email protected]>

Anyway
Acked-by: Michal Hocko <[email protected]>

> ---
>  .../admin-guide/cgroup-v1/memory.rst          | 49 +++----------------
>  mm/memcontrol-v1.c                            | 43 +++++++++-------
>  2 files changed, 32 insertions(+), 60 deletions(-)
> 
> diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
> index 7db63c002922..7d2a44af52c9 100644
> --- a/Documentation/admin-guide/cgroup-v1/memory.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memory.rst
> @@ -47,7 +47,6 @@ Features:
>   - pages are linked to per-memcg LRU exclusively, and there is no global LRU.
>   - optionally, memory+swap usage can be accounted and limited.
>   - hierarchical accounting
> - - soft limit
>   - moving (recharging) account at moving a task is selectable.
>   - usage threshold notifier
>   - memory pressure notifier
> @@ -76,10 +75,9 @@ Brief summary of control files.
>   memory.memsw.failcnt		     show the number of memory+Swap hits limits
>   memory.max_usage_in_bytes	     show max memory usage recorded
>   memory.memsw.max_usage_in_bytes     show max memory+Swap usage recorded
> - memory.soft_limit_in_bytes	     set/show soft limit of memory usage
> -				     This knob is not available on CONFIG_PREEMPT_RT systems.
> -                                     This knob is deprecated and shouldn't be
> -                                     used.
> + memory.soft_limit_in_bytes	     This knob is deprecated and has no effect.
> +                                     Writes are ignored and reads always
> +                                     return the maximum value.
>   memory.stat			     show various statistics
>   memory.use_hierarchy		     set/show hierarchical account enabled
>                                       This knob is deprecated and shouldn't be
> @@ -340,9 +338,6 @@ memory.kmem.usage_in_bytes, or in a separate counter when it makes sense.
>  The main "kmem" counter is fed into the main counter, so kmem charges will
>  also be visible from the user counter.
>  
> -Currently no soft limit is implemented for kernel memory. It is future work
> -to trigger slab reclaim when those limits are reached.
> -
>  2.7.1 Current Kernel Memory resources accounted
>  -----------------------------------------------
>  
> @@ -710,42 +705,10 @@ For compatibility reasons writing 1 to memory.use_hierarchy will always pass::
>  
>  THIS IS DEPRECATED!
>  
> -Soft limits allow for greater sharing of memory. The idea behind soft limits
> -is to allow control groups to use as much of the memory as needed, provided
> -
> -a. There is no memory contention
> -b. They do not exceed their hard limit
> -
> -When the system detects memory contention or low memory, control groups
> -are pushed back to their soft limits. If the soft limit of each control
> -group is very high, they are pushed back as much as possible to make
> -sure that one control group does not starve the others of memory.
> -
> -Please note that soft limits is a best-effort feature; it comes with
> -no guarantees, but it does its best to make sure that when memory is
> -heavily contended for, memory is allocated based on the soft limit
> -hints/setup. Currently soft limit based reclaim is set up such that
> -it gets invoked from balance_pgdat (kswapd).
> -
> -7.1 Interface
> --------------
> -
> -Soft limits can be setup by using the following commands (in this example we
> -assume a soft limit of 256 MiB)::
> -
> -	# echo 256M > memory.soft_limit_in_bytes
> -
> -If we want to change this to 1G, we can at any time use::
> +Writing to memory.soft_limit_in_bytes has no effect and reading it will
> +always return the maximum value.
>  
> -	# echo 1G > memory.soft_limit_in_bytes
> -
> -.. note::
> -       Soft limits take effect over a long period of time, since they involve
> -       reclaiming memory for balancing between memory cgroups
> -
> -.. note::
> -       It is recommended to set the soft limit always below the hard limit,
> -       otherwise the hard limit will take precedence.
> +Use memory.low and memory.min in cgroup v2 instead.
>  
>  .. _cgroup-v1-memory-move-charges:
>  
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index 835fc8e51184..05ef55cae4dc 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -96,7 +96,6 @@ enum {
>  	RES_LIMIT,
>  	RES_MAX_USAGE,
>  	RES_FAILCNT,
> -	RES_SOFT_LIMIT,
>  };
>  
>  #ifdef CONFIG_LOCKDEP
> @@ -1888,6 +1887,30 @@ static int mem_cgroup_hierarchy_write(struct cgroup_subsys_state *css,
>  	return -EINVAL;
>  }
>  
> +static u64 mem_cgroup_soft_limit_read(struct cgroup_subsys_state *css,
> +				      struct cftype *cft)
> +{
> +	return (u64)PAGE_COUNTER_MAX * PAGE_SIZE;
> +}
> +
> +static ssize_t mem_cgroup_soft_limit_write(struct kernfs_open_file *of,
> +					   char *buf, size_t nbytes, loff_t off)
> +{
> +	unsigned long nr_pages;
> +	int ret;
> +
> +	ret = page_counter_memparse(strstrip(buf), "-1", &nr_pages);
> +	if (ret)
> +		return ret;
> +
> +	pr_warn_once("soft_limit_in_bytes is deprecated and will be removed. "
> +		     "Writing any value to this file has no effect. "
> +		     "Please report your usecase to [email protected] if you "
> +		     "depend on this functionality.\n");
> +
> +	return nbytes;
> +}
> +
>  static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
>  			       struct cftype *cft)
>  {
> @@ -1924,8 +1947,6 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
>  		return (u64)counter->watermark * PAGE_SIZE;
>  	case RES_FAILCNT:
>  		return counter->failcnt;
> -	case RES_SOFT_LIMIT:
> -		return (u64)READ_ONCE(memcg->soft_limit) * PAGE_SIZE;
>  	default:
>  		BUG();
>  	}
> @@ -2020,17 +2041,6 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  			break;
>  		}
>  		break;
> -	case RES_SOFT_LIMIT:
> -		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> -			ret = -EOPNOTSUPP;
> -		} else {
> -			pr_warn_once("soft_limit_in_bytes is deprecated and will be removed. "
> -				     "Please report your usecase to [email protected] if you "
> -				     "depend on this functionality.\n");
> -			WRITE_ONCE(memcg->soft_limit, nr_pages);
> -			ret = 0;
> -		}
> -		break;
>  	}
>  	return ret ?: nbytes;
>  }
> @@ -2384,9 +2394,8 @@ struct cftype mem_cgroup_legacy_files[] = {
>  	},
>  	{
>  		.name = "soft_limit_in_bytes",
> -		.private = MEMFILE_PRIVATE(_MEM, RES_SOFT_LIMIT),
> -		.write = mem_cgroup_write,
> -		.read_u64 = mem_cgroup_read_u64,
> +		.write = mem_cgroup_soft_limit_write,
> +		.read_u64 = mem_cgroup_soft_limit_read,
>  	},
>  	{
>  		.name = "failcnt",
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs