Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

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

On Mon, 2011-01-10 at 23:31 +0000, David Rientjes wrote:
> On Mon, 10 Jan 2011, Catalin Marinas wrote:
> > > It would be a shame if the allocation were __GFP_NORETRY and kmemleak
> > > ended up looping forever because it suppresses the bit for a single page,
> > > it uses __GFP_NOMEMALLOC and kmemleak ends up allocating from memory
> > > reserves, or it uses __GFP_HARDWALL and kmemleak is allocating metadata in
> > > a different cpuset.
> > >
> > > I'm not sure why you're not just masking __GFP_NOFAIL and __GFP_REPEAT and
> > > then failing gracefully?  (And __GFP_ZERO and __GFP_COMP, too, of course.)
[...]
> > If the user calling the kernel alloc function cannot get memory,
> > kmemleak won't be called anyway.
> 
> I'm talking about when the allocation is successful and the metadata
> allocation is not, such as what Toralf reported.  If you pass
> __GFP_NOFAIL, it's going to loop forever which is certainly not what we'd
> want: we'd rather just disable kmemleak and continue doing work.  

I agree with this but kmemleak doesn't pass any __GFP_NOFAIL flag (it's
masked out). The reason it only keeps (GFP_KERNEL | GFP_ATOMIC) from the
caller flags is to know whether the kmemleak metadata allocation must be
atomic or not. All the other flags a user may pass like __GFP_NOFAIL are
masked out.

Of course, kmemleak can pass additional flags for allocating its
metadata, but I see this as unrelated to what the user passed (as long
as the atomicity is preserved).

Now, if the user passes __GFP_NOFAIL, do we want such flag for the
kmemleak allocation? IMHO we don't need this (hence it is masked out).
We just allow the kmemleak allocation to fail.

> If you
> pass __GFP_NOMEMALLOC, then kmemleak can allocate from memory reserves in
> the reclaim path which is not allowed.  And if you don't pass
> __GFP_HARDWALL, then these allocations can break memory isolation by
> allowing the metadata to be allocated from different cpusets.

As I said above, we can pass additional flags like __GFP_NOMEMALLOC. But
I think these should be hard-coded for kmemleak allocations irrespective
of the k*alloc user gfp flags.

As for the __GFP_HARDWALL, I don't think it matters much. Kmemleak
doesn't have per-CPU data anyway and the tree for storing the metada is
a global one. When scanning the memory, it does it on a single CPU no
matter where the object was allocated from.
> 
> In other words, I'm pretty sure you don't want to be masking these options
> off of the caller allocation when passed.  It makes no sense for the user
> to do a GFP_KERNEL | __GFP_NORETRY allocation and then have kmemleak loop
> forever.

__GFP_NORETRY is another flag we could force on kmemleak metadata
allocations. See below:


kmemleak: Allow kmemleak metadata allocations to fail

From: Catalin Marinas <catalin.marinas@arm.com>

This patch adds __GFP_NORETRY and __GFP_NOMEMALLOC flags to the kmemleak
metadata allocations so that it has a smaller effect on the users of the
kernel slab allocator. Since kmemleak allocations can now fail more
often, this patch also reduces the verbosity by passing __GFP_NOWARN and
not dumping the stack trace when a kmemleak allocation fails.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: David Rientjes <rientjes@google.com>
Cc: Ted Ts'o <tytso@mit.edu>
---
 mm/kmemleak.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index bd9bc21..84225f3 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -113,7 +113,9 @@
 #define BYTES_PER_POINTER	sizeof(void *)
 
 /* GFP bitmask for kmemleak internal allocations */
-#define GFP_KMEMLEAK_MASK	(GFP_KERNEL | GFP_ATOMIC)
+#define gfp_kmemleak_mask(gfp)	(((gfp) & (GFP_KERNEL | GFP_ATOMIC)) | \
+				 __GFP_NORETRY | __GFP_NOMEMALLOC | \
+				 __GFP_NOWARN)
 
 /* scanning area inside a memory block */
 struct kmemleak_scan_area {
@@ -511,9 +513,10 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	struct kmemleak_object *object;
 	struct prio_tree_node *node;
 
-	object = kmem_cache_alloc(object_cache, gfp & GFP_KMEMLEAK_MASK);
+	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {
-		kmemleak_stop("Cannot allocate a kmemleak_object structure\n");
+		pr_warning("Cannot allocate a kmemleak_object structure\n");
+		kmemleak_disable();
 		return NULL;
 	}
 
@@ -734,9 +737,9 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 		return;
 	}
 
-	area = kmem_cache_alloc(scan_area_cache, gfp & GFP_KMEMLEAK_MASK);
+	area = kmem_cache_alloc(scan_area_cache, gfp_kmemleak_mask(gfp));
 	if (!area) {
-		kmemleak_warn("Cannot allocate a scan area\n");
+		pr_warning("Cannot allocate a scan area\n");
 		goto out;
 	}
 


-- 
Catalin