linux-kernel - Re: [PATCH] memcg, kmem: further deprecate kmem.limit_in

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201118195822.GW12284@dhcp22.suse.cz>
Date:   Wed, 18 Nov 2020 20:58:22 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Shakeel Butt <shakeelb@...gle.com>
Cc:     Roman Gushchin <guro@...com>, Johannes Weiner <hannes@...xchg.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] memcg, kmem: further deprecate kmem.limit_in_bytes

On Wed 18-11-20 09:57:26, Shakeel Butt wrote:
> The deprecation process of kmem.limit_in_bytes started with the commit
> 0158115f702 ("memcg, kmem: deprecate kmem.limit_in_bytes") which also
> explains in detail the motivation behind the deprecation. To summarize,
> it is the unexpected behavior on hitting the kmem limit. This patch
> moves the deprecation process to the next stage by disallowing to set
> the kmem limit. In future we might just remove the kmem.limit_in_bytes
> file completely.
> 
> Signed-off-by: Shakeel Butt <shakeelb@...gle.com>

I am not against this. I am just not sure whether one year is enough for
those users who tend to have a more considervative kernel upgrade path.
I am not worried about SLES user base much as we didn't even enable
KMEM accounting when it was still guarded by a config option. Not sure
about others though.

Considering the code cleanup is not that large, I would rather wait some
more. But you can add
Acked-by: Michal Hocko <mhocko@...e.com>

Maybe we can ask Andrew to put it into mmotm for few releases.

> ---
>  .../admin-guide/cgroup-v1/memory.rst          |  6 ++--
>  mm/memcontrol.c                               | 35 +++----------------
>  2 files changed, 6 insertions(+), 35 deletions(-)
> 
> diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
> index 52688ae34461..cb993d80194d 100644
> --- a/Documentation/admin-guide/cgroup-v1/memory.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memory.rst
> @@ -87,10 +87,8 @@ Brief summary of control files.
>   memory.oom_control		     set/show oom controls.
>   memory.numa_stat		     show the number of memory usage per numa
>  				     node
> - memory.kmem.limit_in_bytes          set/show hard limit for kernel memory
> -                                     This knob is deprecated and shouldn't be
> -                                     used. It is planned that this be removed in
> -                                     the foreseeable future.
> + memory.kmem.limit_in_bytes          This knob is deprecated and writing to
> +                                     it will return -EINVAL.
>   memory.kmem.usage_in_bytes          show current kernel memory allocation
>   memory.kmem.failcnt                 show the number of kernel memory usage
>  				     hits limits
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 45465c03a8d7..78d17b3181ad 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3075,28 +3075,14 @@ static void memcg_free_cache_id(int id)
>  int __memcg_kmem_charge(struct mem_cgroup *memcg, gfp_t gfp,
>  			unsigned int nr_pages)
>  {
> -	struct page_counter *counter;
>  	int ret;
>  
>  	ret = try_charge(memcg, gfp, nr_pages);
>  	if (ret)
>  		return ret;
>  
> -	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
> -	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
> -
> -		/*
> -		 * Enforce __GFP_NOFAIL allocation because callers are not
> -		 * prepared to see failures and likely do not have any failure
> -		 * handling code.
> -		 */
> -		if (gfp & __GFP_NOFAIL) {
> -			page_counter_charge(&memcg->kmem, nr_pages);
> -			return 0;
> -		}
> -		cancel_charge(memcg, nr_pages);
> -		return -ENOMEM;
> -	}
> +	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> +		page_counter_charge(&memcg->kmem, nr_pages);
>  	return 0;
>  }
>  
> @@ -3769,17 +3755,6 @@ static void memcg_free_kmem(struct mem_cgroup *memcg)
>  }
>  #endif /* CONFIG_MEMCG_KMEM */
>  
> -static int memcg_update_kmem_max(struct mem_cgroup *memcg,
> -				 unsigned long max)
> -{
> -	int ret;
> -
> -	mutex_lock(&memcg_max_mutex);
> -	ret = page_counter_set_max(&memcg->kmem, max);
> -	mutex_unlock(&memcg_max_mutex);
> -	return ret;
> -}
> -
>  static int memcg_update_tcp_max(struct mem_cgroup *memcg, unsigned long max)
>  {
>  	int ret;
> @@ -3845,10 +3820,8 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
>  			ret = mem_cgroup_resize_max(memcg, nr_pages, true);
>  			break;
>  		case _KMEM:
> -			pr_warn_once("kmem.limit_in_bytes is deprecated and will be removed. "
> -				     "Please report your usecase to linux-mm@...ck.org if you "
> -				     "depend on this functionality.\n");
> -			ret = memcg_update_kmem_max(memcg, nr_pages);
> +			/* kmem.limit_in_bytes is deprecated. */
> +			ret = -EINVAL;
>  			break;
>  		case _TCP:
>  			ret = memcg_update_tcp_max(memcg, nr_pages);
> -- 
> 2.29.2.299.gdc1121823c-goog

-- 
Michal Hocko
SUSE Labs