On Mon 06-04-20 23:48:54, Yafang Shao wrote:
> When I run my memcg testcase which creates lots of memcgs, I found
> there're unexpected out of memory logs while there're still enough
> available free memory. The error log is,
> mkdir: cannot create directory 'foo.65533': Cannot allocate memory
>
> The reason is when we try to create more than MEM_CGROUP_ID_MAX memcgs, an
> -ENOMEM errno will be set by mem_cgroup_css_alloc(), but the right errno
> should be -EBUSY "Device or resource busy". That is same with
> memcg_alloc_cache_id().
I do not see EBUSY being listed as expected return value for mkdir(2)
which is the primary way to create a cgroup.
> As the errno really misled me, we should make it right. After this patch,
> the error log will be,
> mkdir: cannot create directory 'foo.65533': Device or resource busy
I do see ENOMEM being slightly confusing but if we really need to fix
this then ENOSPC sounds like a better fit to me.
man page says
ENOSPC The new directory cannot be created because the user's disk quota is exhausted.
and that actually matches because id is a form of a quota.
> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> mm/memcontrol.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ca194864d802..94319e4dd1bd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2717,8 +2717,12 @@ static int memcg_alloc_cache_id(void)
>
> id = ida_simple_get(&memcg_cache_ida,
> 0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> - if (id < 0)
> + if (id < 0) {
> + if (id == -ENOSPC)
> + id = -EBUSY;
> +
> return id;
> + }
>
> if (id < memcg_nr_cache_ids)
> return id;
> @@ -4986,19 +4990,26 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
> unsigned int size;
> int node;
> int __maybe_unused i;
> + long error = -ENOMEM;
>
> size = sizeof(struct mem_cgroup);
> size += nr_node_ids * sizeof(struct mem_cgroup_per_node *);
>
> memcg = kzalloc(size, GFP_KERNEL);
> if (!memcg)
> - return NULL;
> + return ERR_PTR(error);
>
> memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL,
> 1, MEM_CGROUP_ID_MAX,
> GFP_KERNEL);
> - if (memcg->id.id < 0)
> + if (memcg->id.id < 0) {
> + if (memcg->id.id == -ENOSPC)
> + error = -EBUSY;
> + else
> + error = memcg->id.id;
> +
> goto fail;
> + }
>
> memcg->vmstats_local = alloc_percpu(struct memcg_vmstats_percpu);
> if (!memcg->vmstats_local)
> @@ -5042,7 +5053,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
> fail:
> mem_cgroup_id_remove(memcg);
> __mem_cgroup_free(memcg);
> - return NULL;
> + return ERR_PTR(error);
> }
>
> static struct cgroup_subsys_state * __ref
> @@ -5053,8 +5064,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> long error = -ENOMEM;
>
> memcg = mem_cgroup_alloc();
> - if (!memcg)
> - return ERR_PTR(error);
> + if (IS_ERR(memcg))
> + return ERR_CAST(memcg);
>
> WRITE_ONCE(memcg->high, PAGE_COUNTER_MAX);
> memcg->soft_limit = PAGE_COUNTER_MAX;
> @@ -5104,7 +5115,7 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> fail:
> mem_cgroup_id_remove(memcg);
> mem_cgroup_free(memcg);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(error);
> }
>
> static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
> --
> 2.18.1
>
--
Michal Hocko
SUSE Labs
