Re: [BUG] kmemcg limit defeats __GFP_NOFAIL allocation

[Michal Hocko <mhocko@kernel.org>]

On Fri 06-09-19 13:54:30, Andrey Ryabinin wrote:
> 
> 
> On 9/6/19 10:27 AM, Michal Hocko wrote:
> > On Fri 06-09-19 01:11:53, Thomas Lindroth wrote:
> >> On 9/4/19 6:39 PM, Tetsuo Handa wrote:
> >>> On 2019/09/04 23:29, Michal Hocko wrote:
> >>>> Ohh, right. We are trying to uncharge something that hasn't been charged
> >>>> because page_counter_try_charge has failed. So the fix needs to be more
> >>>> involved. Sorry, I should have realized that.
> >>>
> >>> OK. Survived the test. Thomas, please try.
> >>>
> >>>> ---
> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>>> index 9ec5e12486a7..e18108b2b786 100644
> >>>> --- a/mm/memcontrol.c
> >>>> +++ b/mm/memcontrol.c
> >>>> @@ -2821,6 +2821,16 @@ int __memcg_kmem_charge_memcg(struct page *page, gfp_t gfp, int order,
> >>>>   	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;
> >>>>   	}
> >>>>
> >>
> >> I tried the patch with 5.2.11 and wasn't able to trigger any null pointer
> >> deref crashes with it. Testing is tricky because the OOM killer will still
> >> run and eventually kill bash and whatever runs in the cgroup.
> > 
> > Yeah, this is unfortunate but also unfixable I am afraid. 
> 
> I think there are two possible ways to fix this. If we decide to keep kmem.limit_in_bytes broken,
> than we can just always bypass limit. Also we could add something like pr_warn_once("kmem limit doesn't work");
> when user changes kmem.limit_in_bytes 
> 
> 
> Or we can fix kmem.limit_in_bytes like this:

I would rather state the brokenness in the documentation. I do not want
to make the more complex. I have only glanced through your patch but
sheer size is really discouraging. Besides that the issue is really not
fixable because kmem charges are simply never going to be guaranteed to
be reclaimable and we simply cannot involve the memcg OOM killer to
resolve the problem. Having a separate counter was just a bad design
choice :/
-- 
Michal Hocko
SUSE Labs