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
