| Message ID | 1586177647-11889-1-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, memcg: fix error return value of mem_cgroup_alloc() | expand |
On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc(). I think idr_alloc() is wrong. I think the right errno to return here is EBUSY "Device or resource busy". > -static struct mem_cgroup *mem_cgroup_alloc(void) > +static struct mem_cgroup *mem_cgroup_alloc(long *error) The normal way to do this is to return an ERR_PTR(). See include/linux/err.h.
On Mon, Apr 6, 2020 at 9:05 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc(). > > I think idr_alloc() is wrong. I think the right errno to return here is > EBUSY "Device or resource busy". > Agree with you that EBUSY is better. I will correct it. > > -static struct mem_cgroup *mem_cgroup_alloc(void) > > +static struct mem_cgroup *mem_cgroup_alloc(long *error) > > The normal way to do this is to return an ERR_PTR(). See > include/linux/err.h. > Thanks for your advise. I will take a look at it. Thanks Yafang
On Mon, Apr 6, 2020 at 9:30 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Mon, Apr 6, 2020 at 9:05 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Mon, Apr 06, 2020 at 08:54:07AM -0400, 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 -ENOSPC, as explained above the function idr_alloc(). > > > > I think idr_alloc() is wrong. I think the right errno to return here is > > EBUSY "Device or resource busy". > > > > Agree with you that EBUSY is better. > I will correct it. > Hi Matthew, What about ida_alloc_range() ? Should we correct it as well ? * Return: The allocated ID, or %-ENOMEM if memory could not be allocated, * or %-ENOSPC if there are no free IDs. */ int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max, gfp_t gfp) If there're no free IDs, ida_alloc_range() also returns ENOSPC. > > > -static struct mem_cgroup *mem_cgroup_alloc(void) > > > +static struct mem_cgroup *mem_cgroup_alloc(long *error) > > > > The normal way to do this is to return an ERR_PTR(). See > > include/linux/err.h. > > > > Thanks for your advise. I will take a look at it. > > Thanks Yafang
On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote: > What about ida_alloc_range() ? Should we correct it as well ? > > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated, > * or %-ENOSPC if there are no free IDs. > */ > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max, > gfp_t gfp) > > If there're no free IDs, ida_alloc_range() also returns ENOSPC. Do you want to check all the callers of ida_.*alloc()?
On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote: > > What about ida_alloc_range() ? Should we correct it as well ? > > > > > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated, > > * or %-ENOSPC if there are no free IDs. > > */ > > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max, > > gfp_t gfp) > > > > If there're no free IDs, ida_alloc_range() also returns ENOSPC. > > Do you want to check all the callers of ida_.*alloc()? Sorry, I can't your point. I just find the mem_cgroup_css_alloc() will use ida too. mem_cgroup_css_alloc memcg_online_kmem memcg_alloc_cache_id ida_simple_get I think we should keep the behavior consistent here. Thanks Yafang
On Mon, Apr 06, 2020 at 10:16:56PM +0800, Yafang Shao wrote: > On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote: > > > What about ida_alloc_range() ? Should we correct it as well ? > > > > > > > > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated, > > > * or %-ENOSPC if there are no free IDs. > > > */ > > > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max, > > > gfp_t gfp) > > > > > > If there're no free IDs, ida_alloc_range() also returns ENOSPC. > > > > Do you want to check all the callers of ida_.*alloc()? > > Sorry, I can't your point. Changing the value returned by ida_alloc_range() would require checking every caller to see if it would break anything. That's why I didn't change it when I rewrote it. > I just find the mem_cgroup_css_alloc() will use ida too. > mem_cgroup_css_alloc > memcg_online_kmem > memcg_alloc_cache_id > ida_simple_get > > I think we should keep the behavior consistent here. ENOSPC doesn't make much sense here. So I'd do: id = ida_simple_get(&memcg_cache_ida, 0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL); + if (id == -ENOSPC) + return -EBUSY; if (id < 0) return id;
On Mon, Apr 6, 2020 at 10:25 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Apr 06, 2020 at 10:16:56PM +0800, Yafang Shao wrote: > > On Mon, Apr 6, 2020 at 10:11 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Mon, Apr 06, 2020 at 10:09:55PM +0800, Yafang Shao wrote: > > > > What about ida_alloc_range() ? Should we correct it as well ? > > > > > > > > > > > > * Return: The allocated ID, or %-ENOMEM if memory could not be allocated, > > > > * or %-ENOSPC if there are no free IDs. > > > > */ > > > > int ida_alloc_range(struct ida *ida, unsigned int min, unsigned int max, > > > > gfp_t gfp) > > > > > > > > If there're no free IDs, ida_alloc_range() also returns ENOSPC. > > > > > > Do you want to check all the callers of ida_.*alloc()? > > > > Sorry, I can't your point. > > Changing the value returned by ida_alloc_range() would require checking > every caller to see if it would break anything. That's why I didn't > change it when I rewrote it. > Got it. Thanks for the explaination. > > I just find the mem_cgroup_css_alloc() will use ida too. > > mem_cgroup_css_alloc > > memcg_online_kmem > > memcg_alloc_cache_id > > ida_simple_get > > > > I think we should keep the behavior consistent here. > > ENOSPC doesn't make much sense here. So I'd do: > > id = ida_simple_get(&memcg_cache_ida, > 0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL); > + if (id == -ENOSPC) > + return -EBUSY; > if (id < 0) > return id; > Understood! Thanks Yafang
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index ca19486..9c85470 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -4980,7 +4980,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg) __mem_cgroup_free(memcg); } -static struct mem_cgroup *mem_cgroup_alloc(void) +static struct mem_cgroup *mem_cgroup_alloc(long *error) { struct mem_cgroup *memcg; unsigned int size; @@ -4997,8 +4997,10 @@ static struct mem_cgroup *mem_cgroup_alloc(void) 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) { + *error = memcg->id.id; goto fail; + } memcg->vmstats_local = alloc_percpu(struct memcg_vmstats_percpu); if (!memcg->vmstats_local) @@ -5052,7 +5054,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void) struct mem_cgroup *memcg; long error = -ENOMEM; - memcg = mem_cgroup_alloc(); + memcg = mem_cgroup_alloc(&error); if (!memcg) return ERR_PTR(error);
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 -ENOSPC, as explained above the function idr_alloc(). 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': No space left on device Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- mm/memcontrol.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)