Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Apr 25, 2018 at 05:50:41AM -0400, Chunyu Hu wrote:
> ----- Original Message -----
> > From: "Catalin Marinas" <catalin.marinas@arm.com>
> > On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> > > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > > [...]
> > > > So if there is a new flag, it would be the 25th bits.
> > > 
> > > No new flags please. Can you simply store a simple bool into
> > > fail_page_alloc
> > > and have save/restore api for that?
> > 
> > For kmemleak, we probably first hit failslab. Something like below may
> > do the trick:
> > 
> > diff --git a/mm/failslab.c b/mm/failslab.c
> > index 1f2f248e3601..63f13da5cb47 100644
> > --- a/mm/failslab.c
> > +++ b/mm/failslab.c
> > @@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
> > gfpflags)
> >  	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
> >  		return false;
> >  
> > +	if (s->flags & SLAB_NOLEAKTRACE)
> > +		return false;
> > +
> >  	return should_fail(&failslab.attr, s->object_size);
> >  }
> 
> This maybe is the easy enough way for skipping fault injection for
> kmemleak slab object. 

This was added to avoid kmemleak tracing itself, so could be used for
other kmemleak-related cases.

> > Can we get a second should_fail() via should_fail_alloc_page() if a new
> > slab page is allocated?
> 
> looking at code path below, what do you mean by getting a second
> should_fail() via fail_alloc_page?

Kmemleak calls kmem_cache_alloc() on a cache with SLAB_LEAKNOTRACE, so the
first point of failure injection is __should_failslab() which we can
handle with the slab flag. The slab allocator itself ends up calling
alloc_pages() to allocate a slab page (and __GFP_NOFAIL is explicitly
cleared). Here we have the second potential failure injection via
fail_alloc_page(). That's unless the order < fail_page_alloc.min_order
which I think is the default case (min_order = 1 while the slab page
allocation for kmemleak would need an order of 0. It's not ideal but we
may get away with it.

> Seems we need to insert the flag between alloc_slab_page and
> alloc_pages()? Without GFP flag, it's difficult to pass info to
> should_fail_alloc_page and keep simple at same time. 

Indeed.

> Or as Michal suggested, completely disabling page alloc fail injection
> when kmemleak enabled. And enable it again when kmemleak off. 

Dmitry's point was that kmemleak is still useful to detect leaks on the
error path where errors are actually introduced by the fault injection.
Kmemleak cannot cope with allocation failures as it needs a pretty
precise tracking of the allocated objects.

An alternative could be to not free the early_log buffer in kmemleak and
use that memory in an emergency when allocation fails (though I don't
particularly like this).

Yet another option is to use NOFAIL and remove NORETRY in kmemleak when
fault injection is enabled.

-- 
Catalin