| Message ID | 20201118175726.2453120-1-shakeelb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memcg, kmem: further deprecate kmem.limit_in_bytes | expand |
On Wed, Nov 18, 2020 at 09:57:26AM -0800, 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@google.com> The first stage was done over a year ago, so if there were no complains it feels like it's a good time to move forward. Acked-by: Roman Gushchin <guro@fb.com> The only question I have is if it's better to return -EINVAL or -ENOTSUPP. The latter option could be more convenient for userspace, because it will be clear that the kernel is not supporting the functionality, rather than the passed value is incorrect (e.g. if the value is read from a config, provided by a user). I'm not sure though, just an idea. Thanks!
On Wed, Nov 18, 2020 at 11:46 AM Roman Gushchin <guro@fb.com> wrote: > > On Wed, Nov 18, 2020 at 09:57:26AM -0800, 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@google.com> > > The first stage was done over a year ago, so if there were no complains > it feels like it's a good time to move forward. > > Acked-by: Roman Gushchin <guro@fb.com> Thanks. > > The only question I have is if it's better to return -EINVAL or -ENOTSUPP. > The latter option could be more convenient for userspace, because it will > be clear that the kernel is not supporting the functionality, rather than > the passed value is incorrect (e.g. if the value is read from a config, provided > by a user). I'm not sure though, just an idea. > Let's see what others say. I am ok with -ENOTSUPP as well.
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@google.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@suse.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@kvack.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
On Wed, Nov 18, 2020 at 11:58 AM Michal Hocko <mhocko@suse.com> wrote: > > 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@google.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 was thinking of removing the kmem page counter in the followup but thought of sending this alone to see if now is the right time. > I would rather wait some > more. But you can add > Acked-by: Michal Hocko <mhocko@suse.com> > > Maybe we can ask Andrew to put it into mmotm for few releases. > Ok with me. I will send the full series and will ask Andrew to keep the series in mm tree for a couple of releases.
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@kvack.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);
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@google.com> --- .../admin-guide/cgroup-v1/memory.rst | 6 ++-- mm/memcontrol.c | 35 +++---------------- 2 files changed, 6 insertions(+), 35 deletions(-)