Re: [BUG] kmemcg limit defeats __GFP_NOFAIL allocation

[Michal Hocko <mhocko@kernel.org>]

On Wed 04-09-19 20:32:25, Tetsuo Handa wrote:
> On 2019/09/04 20:25, Michal Hocko wrote:
> > On Wed 04-09-19 18:36:06, Tetsuo Handa wrote:
> > [...]
> >> The first bug is that __memcg_kmem_charge_memcg() in mm/memcontrol.c is
> >> failing to return 0 when it is a __GFP_NOFAIL allocation request.
> >> We should ignore limits when it is a __GFP_NOFAIL allocation request.
> > 
> > OK, fixing that sounds like a reasonable thing to do.
> >  
> >> If we force __memcg_kmem_charge_memcg() to return 0, then
> >>
> >> ----------
> >>         struct page_counter *counter;
> >>         int ret;
> >>
> >> +       if (gfp & __GFP_NOFAIL)
> >> +               return 0;
> >> +
> >>         ret = try_charge(memcg, gfp, nr_pages);
> >>         if (ret)
> >>                 return ret;
> >> ----------
> > 
> > This should be more likely something like
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 9ec5e12486a7..05a4828edf9d 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2820,7 +2820,8 @@ int __memcg_kmem_charge_memcg(struct page *page, gfp_t gfp, int order,
> >  		return ret;
> >  
> >  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
> > -	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
> > +	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter) &&
> > +	    !(gfp_mask & __GFP_NOFAIL)) {
> >  		cancel_charge(memcg, nr_pages);
> >  		return -ENOMEM;
> >  	}
> 
> Is it guaranteed that try_charge(__GFP_NOFAIL) never fails?

it enforces charges.

> >> the second bug that alloc_slabmgmt() in mm/slab.c is returning NULL
> >> when it is a __GFP_NOFAIL allocation request will appear.
> >> I don't know how to handle this.
> > 
> > I am sorry, I do not follow, why would alloc_slabmgmt return NULL
> > with forcing gfp_nofail charges?
> > 
> 
> The reproducer is hitting
> 
> @@ -2300,18 +2302,21 @@ static void *alloc_slabmgmt(struct kmem_cache *cachep,
>  	page->s_mem = addr + colour_off;
>  	page->active = 0;
>  
> -	if (OBJFREELIST_SLAB(cachep))
> +	if (OBJFREELIST_SLAB(cachep)) {
> +		BUG_ON(local_flags & __GFP_NOFAIL); // <= this condition

What does this bugon tries to say though. I am not an expert on slab bu
only OFF_SLAB(cachep) branch depends on an allocation. Others should
allocate object from the cache.
-- 
Michal Hocko
SUSE Labs