linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
@ 2019-07-27 13:23 Catalin Marinas
  2019-07-30 19:57 ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Catalin Marinas @ 2019-07-27 13:23 UTC (permalink / raw)
  To: linux-mm
  Cc: linux-kernel, Andrew Morton, Michal Hocko, Matthew Wilcox, Qian Cai

Add mempool allocations for struct kmemleak_object and
kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
under memory pressure. Additionally, mask out all the gfp flags passed
to kmemleak other than GFP_KERNEL|GFP_ATOMIC.

A boot-time tuning parameter (kmemleak.mempool) is added to allow a
different minimum pool size (defaulting to NR_CPUS * 4).

Cc: Michal Hocko <mhocko@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Qian Cai <cai@lca.pw>
Suggested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---

v1 posted here:

http://lkml.kernel.org/r/20190328145917.GC10283@arrakis.emea.arm.com

Changes in v2:

- kmemleak.mempool cmdline parameter to configure the minimum pool size
- rebased against -next (on top of the __GFP_NOFAIL revert)

 .../admin-guide/kernel-parameters.txt         |  6 ++
 mm/kmemleak.c                                 | 58 +++++++++++++++----
 2 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 46b826fcb5ad..11c413e3c42b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2011,6 +2011,12 @@
 			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
 			the default is off.
 
+	kmemleak.mempool=
+			[KNL] Boot-time tuning of the minimum kmemleak
+			metadata pool size.
+			Format: <int>
+			Default: NR_CPUS * 4
+
 	kprobe_event=[probe-list]
 			[FTRACE] Add kprobe events and enable at boot time.
 			The probe-list is a semicolon delimited list of probe
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 6e9e8cca663e..a31eab79bcf5 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -69,6 +69,7 @@
 #include <linux/kthread.h>
 #include <linux/rbtree.h>
 #include <linux/fs.h>
+#include <linux/mempool.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/cpumask.h>
@@ -112,9 +113,7 @@
 #define BYTES_PER_POINTER	sizeof(void *)
 
 /* GFP bitmask for kmemleak internal allocations */
-#define gfp_kmemleak_mask(gfp)	(((gfp) & (GFP_KERNEL | GFP_ATOMIC)) | \
-				 __GFP_NORETRY | __GFP_NOMEMALLOC | \
-				 __GFP_NOWARN)
+#define gfp_kmemleak_mask(gfp)	((gfp) & (GFP_KERNEL | GFP_ATOMIC))
 
 /* scanning area inside a memory block */
 struct kmemleak_scan_area {
@@ -190,7 +189,13 @@ static DEFINE_RWLOCK(kmemleak_lock);
 
 /* allocation caches for kmemleak internal data */
 static struct kmem_cache *object_cache;
+static mempool_t *object_mempool;
 static struct kmem_cache *scan_area_cache;
+static mempool_t *scan_area_mempool;
+
+/* default minimum memory pool sizes */
+static int min_object_pool = NR_CPUS * 4;
+static int min_scan_area_pool = NR_CPUS * 1;
 
 /* set if tracing memory operations is enabled */
 static int kmemleak_enabled;
@@ -465,9 +470,9 @@ static void free_object_rcu(struct rcu_head *rcu)
 	 */
 	hlist_for_each_entry_safe(area, tmp, &object->area_list, node) {
 		hlist_del(&area->node);
-		kmem_cache_free(scan_area_cache, area);
+		mempool_free(area, scan_area_mempool);
 	}
-	kmem_cache_free(object_cache, object);
+	mempool_free(object, object_mempool);
 }
 
 /*
@@ -550,7 +555,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	struct rb_node **link, *rb_parent;
 	unsigned long untagged_ptr;
 
-	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
+	object = mempool_alloc(object_mempool, gfp_kmemleak_mask(gfp));
 	if (!object) {
 		pr_warn("Cannot allocate a kmemleak_object structure\n");
 		kmemleak_disable();
@@ -614,7 +619,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 			 * be freed while the kmemleak_lock is held.
 			 */
 			dump_object_info(parent);
-			kmem_cache_free(object_cache, object);
+			mempool_free(object, object_mempool);
 			object = NULL;
 			goto out;
 		}
@@ -772,7 +777,7 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 		return;
 	}
 
-	area = kmem_cache_alloc(scan_area_cache, gfp_kmemleak_mask(gfp));
+	area = mempool_alloc(scan_area_mempool, gfp_kmemleak_mask(gfp));
 	if (!area) {
 		pr_warn("Cannot allocate a scan area\n");
 		goto out;
@@ -784,7 +789,7 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 	} else if (ptr + size > object->pointer + object->size) {
 		kmemleak_warn("Scan area larger than object 0x%08lx\n", ptr);
 		dump_object_info(object);
-		kmem_cache_free(scan_area_cache, area);
+		mempool_free(area, scan_area_mempool);
 		goto out_unlock;
 	}
 
@@ -1993,6 +1998,27 @@ static int __init kmemleak_boot_config(char *str)
 }
 early_param("kmemleak", kmemleak_boot_config);
 
+/*
+ * Allow boot-time tuning of the kmemleak mempool size.
+ */
+static int __init kmemleak_mempool_config(char *str)
+{
+	int size, ret;
+
+	if (!str)
+		return -EINVAL;
+
+	ret = kstrtoint(str, 0, &size);
+	if (ret)
+		return ret;
+
+	min_object_pool = size;
+	min_scan_area_pool = size / 4;
+
+	return 0;
+}
+early_param("kmemleak.mempool", kmemleak_mempool_config);
+
 static void __init print_log_trace(struct early_log *log)
 {
 	pr_notice("Early log backtrace:\n");
@@ -2020,6 +2046,18 @@ void __init kmemleak_init(void)
 
 	object_cache = KMEM_CACHE(kmemleak_object, SLAB_NOLEAKTRACE);
 	scan_area_cache = KMEM_CACHE(kmemleak_scan_area, SLAB_NOLEAKTRACE);
+	if (!object_cache || !scan_area_cache) {
+		kmemleak_disable();
+		return;
+	}
+	object_mempool = mempool_create_slab_pool(min_object_pool,
+						  object_cache);
+	scan_area_mempool = mempool_create_slab_pool(min_scan_area_pool,
+						     scan_area_cache);
+	if (!object_mempool || !scan_area_mempool) {
+		kmemleak_disable();
+		return;
+	}
 
 	if (crt_early_log > ARRAY_SIZE(early_log))
 		pr_warn("Early log buffer exceeded (%d), please increase DEBUG_KMEMLEAK_EARLY_LOG_SIZE\n",
@@ -2126,7 +2164,7 @@ static int __init kmemleak_late_init(void)
 		mutex_unlock(&scan_mutex);
 	}
 
-	pr_info("Kernel memory leak detector initialized\n");
+	pr_info("Kernel memory leak detector initialized (mempool size: %d)\n", min_object_pool);
 
 	return 0;
 }

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-27 13:23 [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects Catalin Marinas
@ 2019-07-30 19:57 ` Andrew Morton
  2019-07-30 20:22   ` Qian Cai
  2019-07-31  9:06   ` Michal Hocko
  2019-07-30 20:02 ` Andrew Morton
  2019-07-31  9:10 ` Michal Hocko
  2 siblings, 2 replies; 15+ messages in thread
From: Andrew Morton @ 2019-07-30 19:57 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox, Qian Cai

On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> Add mempool allocations for struct kmemleak_object and
> kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> under memory pressure. Additionally, mask out all the gfp flags passed
> to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> 
> A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> different minimum pool size (defaulting to NR_CPUS * 4).

Why would anyone ever want to alter this?  Is there some particular
misbehaviour which this will improve?  If so, what is it?

> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2011,6 +2011,12 @@
>  			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
>  			the default is off.
>  
> +	kmemleak.mempool=
> +			[KNL] Boot-time tuning of the minimum kmemleak
> +			metadata pool size.
> +			Format: <int>
> +			Default: NR_CPUS * 4
> +

This is the only documentation we provide people and it doesn't really
explain anything at all.  IOW, can we do a better job of explaining all this
to the target audience?

Why does the min size need to be tunable anyway?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-27 13:23 [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects Catalin Marinas
  2019-07-30 19:57 ` Andrew Morton
@ 2019-07-30 20:02 ` Andrew Morton
  2019-07-31 15:44   ` Catalin Marinas
  2019-07-31  9:10 ` Michal Hocko
  2 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2019-07-30 20:02 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox, Qian Cai

On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> Add mempool allocations for struct kmemleak_object and
> kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> under memory pressure. Additionally, mask out all the gfp flags passed
> to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> 
> A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> different minimum pool size (defaulting to NR_CPUS * 4).

btw, the checkpatch warnings are valid:

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
#70: FILE: mm/kmemleak.c:197:
+static int min_object_pool = NR_CPUS * 4;

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
#71: FILE: mm/kmemleak.c:198:
+static int min_scan_area_pool = NR_CPUS * 1;

There can be situations where NR_CPUS is much larger than
num_possible_cpus().  Can we initialize these tunables within
kmemleak_init()?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-30 19:57 ` Andrew Morton
@ 2019-07-30 20:22   ` Qian Cai
  2019-07-30 20:39     ` Andrew Morton
  2019-07-31  9:53     ` Catalin Marinas
  2019-07-31  9:06   ` Michal Hocko
  1 sibling, 2 replies; 15+ messages in thread
From: Qian Cai @ 2019-07-30 20:22 UTC (permalink / raw)
  To: Andrew Morton, Catalin Marinas
  Cc: linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox

On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
> On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com>
> wrote:
> 
> > Add mempool allocations for struct kmemleak_object and
> > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > under memory pressure. Additionally, mask out all the gfp flags passed
> > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > 
> > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > different minimum pool size (defaulting to NR_CPUS * 4).
> 
> Why would anyone ever want to alter this?  Is there some particular
> misbehaviour which this will improve?  If so, what is it?

So it can tolerant different systems and workloads. For example, there are some
machines with slow disk and fast CPUs. When they are under memory pressure, it
could take a long time to swap before the OOM kicks in to free up some memory.
As the results, it needs a large mempool for kmemleak or suffering from higher
chance of a kmemleak metadata allocation failure.

> 
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2011,6 +2011,12 @@
> >  			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
> >  			the default is off.
> >  
> > +	kmemleak.mempool=
> > +			[KNL] Boot-time tuning of the minimum kmemleak
> > +			metadata pool size.
> > +			Format: <int>
> > +			Default: NR_CPUS * 4
> > +

Catalin, BTW, it is right now unable to handle a large size. I tried to reserve
64M (kmemleak.mempool=67108864),

[    0.039254][    T0] WARNING: CPU: 0 PID: 0 at mm/page_alloc.c:4707
__alloc_pages_nodemask+0x3b8/0x1780
[    0.039284][    T0] Modules linked in:
[    0.039309][    T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.3.0-rc2-next-
20190730+ #3
[    0.039328][    T0] NIP:  c000000000395038 LR: c0000000003d9320 CTR:
0000000000000000
[    0.039355][    T0] REGS: c00000000170f710 TRAP: 0700   Not tainted  (5.3.0-
rc2-next-20190730+)
[    0.039384][    T0] MSR:  9000000002029033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE>  CR:
24000884  XER: 20040000
[    0.039431][    T0] CFAR: c000000000394cd4 IRQMASK: 0 
[    0.039431][    T0] GPR00: c0000000003d9320 c00000000170f9a0 c000000001708c00
0000000000040cc0 
[    0.039431][    T0] GPR04: 0000000000000010 0000000000000000 0000000000000000
c000000002aac080 
[    0.039431][    T0] GPR08: 0000001ffb3a0000 0000000000000000 c0000000003d9320
0000000000000000 
[    0.039431][    T0] GPR12: 0000000024000882 c000000002760000 0000000000000000
0000000000000000 
[    0.039431][    T0] GPR16: 0000000000000000 0000000000000000 0000000000000000
0000000000000000 
[    0.039431][    T0] GPR20: 0000000000000000 0000000000000001 0000000010004d9c
00000000100053ed 
[    0.039431][    T0] GPR24: ffffffffffffffff ffffffffffffffff c0000000002e9544
0000000100000000 
[    0.039431][    T0] GPR28: 0000000000000cc0 0000000100000000 0000000000040cc0
c0000000027e8c48 
[    0.039646][    T0] NIP [c000000000395038]
__alloc_pages_nodemask+0x3b8/0x1780
[    0.039693][    T0] LR [c0000000003d9320] kmalloc_large_node+0x100/0x1a0
[    0.039727][    T0] Call Trace:
[    0.039749][    T0] [c00000000170f9a0] [0000000000000001] 0x1 (unreliable)
[    0.039776][    T0] [c00000000170fbe0] [0000000000000000] 0x0
[    0.039795][    T0] [c00000000170fc80] [c0000000003e5080]
__kmalloc_node+0x520/0x890
[    0.039816][    T0] [c00000000170fd20] [c0000000002e9544]
mempool_init_node+0xb4/0x1e0
[    0.039836][    T0] [c00000000170fd80] [c0000000002e975c]
mempool_create_node+0xcc/0x150
[    0.039857][    T0] [c00000000170fdf0] [c000000000b2a730]
kmemleak_init+0x16c/0x54c
[    0.039878][    T0] [c00000000170fef0] [c000000000ae460c]
start_kernel+0x69c/0x7cc
[    0.039908][    T0] [c00000000170ff90] [c00000000000a7d4]
start_here_common+0x1c/0x434
[    0.039945][    T0] Instruction dump:
[    0.039976][    T0] 4bffff14 e92d0968 39291020 3bc00001 f9210148 4bfffd98
7d435378 4bf94eed 
[    0.040012][    T0] 60000000 4bfffdfc 70692000 4082ffd0 <0fe00000> 3bc00000
4bfffedc 39200000 
[    0.040049][    T0] ---[ end trace 038320b411324ff7 ]---
[    0.040100][    T0] kmemleak: Kernel memory leak detector disabled


[   16.192449][    T1] BUG: Unable to handle kernel data access at
0xffffffffffffb2aa
[   16.192473][    T1] Faulting instruction address: 0xc000000000b2a2fc
[   16.192500][    T1] Oops: Kernel access of bad area, sig: 11 [#1]
[   16.192526][    T1] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=256
DEBUG_PAGEALLOC NUMA PowerNV
[   16.192567][    T1] Modules linked in:
[   16.192593][    T1] CPU: 4 PID: 1 Comm: swapper/0 Tainted:
G        W         5.3.0-rc2-next-20190730+ #3
[   16.192646][    T1] NIP:  c000000000b2a2fc LR: c0000000003e6e48 CTR:
c0000000000b4380
[   16.192698][    T1] REGS: c00000002aaef9d0 TRAP: 0380   Tainted:
G        W          (5.3.0-rc2-next-20190730+)
[   16.192750][    T1] MSR:  9000000002009033 <SF,HV,VEC,EE,ME,IR,DR,RI,LE>  CR:
28002884  XER: 20040000
[   16.192801][    T1] CFAR: c00000000043769c IRQMASK: 0 
[   16.192801][    T1] GPR00: c0000000003e6e48 c00000002aaefc60 c000000001708c00
0000000000000002 
[   16.192801][    T1] GPR04: c000000002c42648 0000000000000000 0000000000000000
ffffffff00001e77 
[   16.192801][    T1] GPR08: 0000000000000000 0000000000000001 0000000000000800
0000000000000000 
[   16.192801][    T1] GPR12: 0000000000002000 c000001fffffbc00 c0000000000103d8
0000000000000000 
[   16.192801][    T1] GPR16: 0000000000000000 0000000000000000 0000000000000000
0000000000000000 
[   16.192801][    T1] GPR20: 0000000000000000 0000000000000000 0000000000000000
0000000000000000 
[   16.192801][    T1] GPR24: 0000000000000000 c000000002aa9c80 c0000000018d0730
c0000000003c9270 
[   16.192801][    T1] GPR28: 000000000000b100 c00c00000000b100 c000000002c42648
c000000002aa9c80 
[   16.193126][    T1] NIP [c000000000b2a2fc] log_early+0x8/0x160
[   16.193153][    T1] LR [c0000000003e6e48] kmem_cache_free+0x428/0x740
[   16.193190][    T1] Call Trace:
[   16.193213][    T1] [c00000002aaefc60] [0000000000000366] 0x366 (unreliable)
[   16.193243][    T1] [c00000002aaefd00] [c0000000003c9270]
__mpol_put+0x50/0x70
[   16.193272][    T1] [c00000002aaefd20] [c0000000003c9488]
do_set_mempolicy+0x108/0x170
[   16.193314][    T1] [c00000002aaefdb0] [c000000000010434]
kernel_init+0x64/0x150
[   16.193363][    T1] [c00000002aaefe20] [c00000000000b1cc]
ret_from_kernel_thread+0x5c/0x70
[   16.193412][    T1] Instruction dump:
[   16.193436][    T1] aaaaaaaa aaaaaaaa aaaaaaaa aaaaaaaa aaaaaaaa aaaaaaaa
aaaaaaaa aaaaaaaa 
[   16.193486][    T1] aaaaaaaa aaaaaaaa aaaaaaaa aaaaaaaa <aaaaaaaa> aaaaaaaa
aaaaaaaa aaaaaaaa 
[   16.193556][    T1] ---[ end trace 038320b411324ff9 ]---
[   16.587204][    T1] 
[   17.587316][    T1] Kernel panic - not syncing: Fatal exception

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-30 20:22   ` Qian Cai
@ 2019-07-30 20:39     ` Andrew Morton
  2019-07-31  9:53     ` Catalin Marinas
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Morton @ 2019-07-30 20:39 UTC (permalink / raw)
  To: Qian Cai
  Cc: Catalin Marinas, linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox

On Tue, 30 Jul 2019 16:22:37 -0400 Qian Cai <cai@lca.pw> wrote:

> On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
> > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com>
> > wrote:
> > 
> > > Add mempool allocations for struct kmemleak_object and
> > > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > > under memory pressure. Additionally, mask out all the gfp flags passed
> > > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > > 
> > > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > > different minimum pool size (defaulting to NR_CPUS * 4).
> > 
> > Why would anyone ever want to alter this?  Is there some particular
> > misbehaviour which this will improve?  If so, what is it?
> 
> So it can tolerant different systems and workloads. For example, there are some
> machines with slow disk and fast CPUs. When they are under memory pressure, it
> could take a long time to swap before the OOM kicks in to free up some memory.
> As the results, it needs a large mempool for kmemleak or suffering from higher
> chance of a kmemleak metadata allocation failure.

This sort of thing should be in the changelog and in the user-facing
documentation please.  Also, we should document the user-visible
effects of this failure so that users can determine whether this tunable
will help them.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-30 19:57 ` Andrew Morton
  2019-07-30 20:22   ` Qian Cai
@ 2019-07-31  9:06   ` Michal Hocko
  2019-07-31  9:17     ` Catalin Marinas
  1 sibling, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2019-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Catalin Marinas, linux-mm, linux-kernel, Matthew Wilcox, Qian Cai

On Tue 30-07-19 12:57:43, Andrew Morton wrote:
> On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > Add mempool allocations for struct kmemleak_object and
> > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > under memory pressure. Additionally, mask out all the gfp flags passed
> > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > 
> > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > different minimum pool size (defaulting to NR_CPUS * 4).
> 
> Why would anyone ever want to alter this?  Is there some particular
> misbehaviour which this will improve?  If so, what is it?

I do agree with Andrew here. Can we simply go with no tunning for now
and only add it based on some real life reports that the auto-tuning is
not sufficient?

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-27 13:23 [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects Catalin Marinas
  2019-07-30 19:57 ` Andrew Morton
  2019-07-30 20:02 ` Andrew Morton
@ 2019-07-31  9:10 ` Michal Hocko
  2 siblings, 0 replies; 15+ messages in thread
From: Michal Hocko @ 2019-07-31  9:10 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: linux-mm, linux-kernel, Andrew Morton, Matthew Wilcox, Qian Cai

On Sat 27-07-19 14:23:33, Catalin Marinas wrote:
> Add mempool allocations for struct kmemleak_object and
> kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> under memory pressure. Additionally, mask out all the gfp flags passed
> to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> 
> A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> different minimum pool size (defaulting to NR_CPUS * 4).
> 
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Qian Cai <cai@lca.pw>
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

I am not familiar with the kmemleak code so I cannot really give my ack
but I can give my thumbs up at least. This is definitely an improvement
and step into the right direction. The gfp flags games were just broken.

My only recommendation would be to drop the kernel parameter as
mentioned in other email. We have just too many of them and if the
current auto-tuning is not sufficient we want to hear about that and
find a better one or add a parameter only if we fail.

Thanks!
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-31  9:06   ` Michal Hocko
@ 2019-07-31  9:17     ` Catalin Marinas
  0 siblings, 0 replies; 15+ messages in thread
From: Catalin Marinas @ 2019-07-31  9:17 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, Matthew Wilcox, Qian Cai

On Wed, Jul 31, 2019 at 11:06:53AM +0200, Michal Hocko wrote:
> On Tue 30-07-19 12:57:43, Andrew Morton wrote:
> > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > Add mempool allocations for struct kmemleak_object and
> > > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > > under memory pressure. Additionally, mask out all the gfp flags passed
> > > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > > 
> > > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > > different minimum pool size (defaulting to NR_CPUS * 4).
> > 
> > Why would anyone ever want to alter this?  Is there some particular
> > misbehaviour which this will improve?  If so, what is it?
> 
> I do agree with Andrew here. Can we simply go with no tunning for now
> and only add it based on some real life reports that the auto-tuning is
> not sufficient?

In a first attempt earlier this year, Qian reported that an emergency
pool (subsequently converted to using mempool) with the default pre-fill
does not help under memory pressure:

https://lore.kernel.org/linux-mm/49f77efc-8375-8fc8-aa89-9814bfbfe5bc@lca.pw/

I'm waiting for him to confirm whether the tunable in this patch helps,
otherwise we can look elsewhere, maybe refilling the mempool via other
means than just on free.

In general, not sure we can do much under memory pressure. I'm looking
at adding the kmemleak metadata to the slab itself (though I get some
weird -EEXIST error in kobject_add_internal) but there are still places
where the metadata needs to be allocated directly and, under OOM, this
is prone to failure. I guess we'll have to live with this.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-30 20:22   ` Qian Cai
  2019-07-30 20:39     ` Andrew Morton
@ 2019-07-31  9:53     ` Catalin Marinas
  2019-07-31 12:02       ` Qian Cai
  1 sibling, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2019-07-31  9:53 UTC (permalink / raw)
  To: Qian Cai
  Cc: Andrew Morton, linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox

On Tue, Jul 30, 2019 at 04:22:37PM -0400, Qian Cai wrote:
> On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
> > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com>
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -2011,6 +2011,12 @@
> > >  			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
> > >  			the default is off.
> > >  
> > > +	kmemleak.mempool=
> > > +			[KNL] Boot-time tuning of the minimum kmemleak
> > > +			metadata pool size.
> > > +			Format: <int>
> > > +			Default: NR_CPUS * 4
> > > +
> 
> Catalin, BTW, it is right now unable to handle a large size. I tried to reserve
> 64M (kmemleak.mempool=67108864),
> 
> [    0.039254][    T0] WARNING: CPU: 0 PID: 0 at mm/page_alloc.c:4707 __alloc_pages_nodemask+0x3b8/0x1780
[...]
> [    0.039646][    T0] NIP [c000000000395038] __alloc_pages_nodemask+0x3b8/0x1780
> [    0.039693][    T0] LR [c0000000003d9320] kmalloc_large_node+0x100/0x1a0
> [    0.039727][    T0] Call Trace:
> [    0.039795][    T0] [c00000000170fc80] [c0000000003e5080] __kmalloc_node+0x520/0x890
> [    0.039816][    T0] [c00000000170fd20] [c0000000002e9544] mempool_init_node+0xb4/0x1e0
> [    0.039836][    T0] [c00000000170fd80] [c0000000002e975c] mempool_create_node+0xcc/0x150
> [    0.039857][    T0] [c00000000170fdf0] [c000000000b2a730] kmemleak_init+0x16c/0x54c
> [    0.039878][    T0] [c00000000170fef0] [c000000000ae460c] start_kernel+0x69c/0x7cc
> [    0.039908][    T0] [c00000000170ff90] [c00000000000a7d4] start_here_common+0x1c/0x434
[...]
> [    0.040100][    T0] kmemleak: Kernel memory leak detector disabled

It looks like the mempool cannot be created. 64M objects means a
kmalloc(512MB) for the pool array in mempool_init_node(), so that hits
the MAX_ORDER warning in __alloc_pages_nodemask().

Maybe the mempool tunable won't help much for your case if you need so
many objects. It's still worth having a mempool for kmemleak but we
could look into changing the refill logic while keeping the original
size constant (say 1024 objects).

> [   16.192449][    T1] BUG: Unable to handle kernel data access at 0xffffffffffffb2aa

This doesn't seem kmemleak related from the trace.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-31  9:53     ` Catalin Marinas
@ 2019-07-31 12:02       ` Qian Cai
  2019-07-31 14:48         ` Catalin Marinas
  0 siblings, 1 reply; 15+ messages in thread
From: Qian Cai @ 2019-07-31 12:02 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Andrew Morton, Linux-MM, linux-kernel, Michal Hocko, Matthew Wilcox



> On Jul 31, 2019, at 5:53 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Tue, Jul 30, 2019 at 04:22:37PM -0400, Qian Cai wrote:
>> On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
>>> On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com>
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -2011,6 +2011,12 @@
>>>>  			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
>>>>  			the default is off.
>>>>  
>>>> +	kmemleak.mempool=
>>>> +			[KNL] Boot-time tuning of the minimum kmemleak
>>>> +			metadata pool size.
>>>> +			Format: <int>
>>>> +			Default: NR_CPUS * 4
>>>> +
>> 
>> Catalin, BTW, it is right now unable to handle a large size. I tried to reserve
>> 64M (kmemleak.mempool=67108864),
>> 
>> [    0.039254][    T0] WARNING: CPU: 0 PID: 0 at mm/page_alloc.c:4707 __alloc_pages_nodemask+0x3b8/0x1780
> [...]
>> [    0.039646][    T0] NIP [c000000000395038] __alloc_pages_nodemask+0x3b8/0x1780
>> [    0.039693][    T0] LR [c0000000003d9320] kmalloc_large_node+0x100/0x1a0
>> [    0.039727][    T0] Call Trace:
>> [    0.039795][    T0] [c00000000170fc80] [c0000000003e5080] __kmalloc_node+0x520/0x890
>> [    0.039816][    T0] [c00000000170fd20] [c0000000002e9544] mempool_init_node+0xb4/0x1e0
>> [    0.039836][    T0] [c00000000170fd80] [c0000000002e975c] mempool_create_node+0xcc/0x150
>> [    0.039857][    T0] [c00000000170fdf0] [c000000000b2a730] kmemleak_init+0x16c/0x54c
>> [    0.039878][    T0] [c00000000170fef0] [c000000000ae460c] start_kernel+0x69c/0x7cc
>> [    0.039908][    T0] [c00000000170ff90] [c00000000000a7d4] start_here_common+0x1c/0x434
> [...]
>> [    0.040100][    T0] kmemleak: Kernel memory leak detector disabled
> 
> It looks like the mempool cannot be created. 64M objects means a
> kmalloc(512MB) for the pool array in mempool_init_node(), so that hits
> the MAX_ORDER warning in __alloc_pages_nodemask().
> 
> Maybe the mempool tunable won't help much for your case if you need so
> many objects. It's still worth having a mempool for kmemleak but we
> could look into changing the refill logic while keeping the original
> size constant (say 1024 objects).

Actually, kmemleak.mempool=524288 works quite well on systems I have here. This
is more of making the code robust by error-handling a large value without the
NULL-ptr-deref below. Maybe simply just validate the value by adding upper bound
to not trigger that warning with MAX_ORDER.

> 
>> [   16.192449][    T1] BUG: Unable to handle kernel data access at 0xffffffffffffb2aa
> 
> This doesn't seem kmemleak related from the trace.

This only happens when passing a large kmemleak.mempool, e.g., 64M

[   16.193126][    T1] NIP [c000000000b2a2fc] log_early+0x8/0x160
[   16.193153][    T1] LR [c0000000003e6e48] kmem_cache_free+0x428/0x740
[   16.193190][    T1] Call Trace:
[   16.193213][    T1] [c00000002aaefc60] [0000000000000366] 0x366 (unreliable)
[   16.193243][    T1] [c00000002aaefd00] [c0000000003c9270]
__mpol_put+0x50/0x70
[   16.193272][    T1] [c00000002aaefd20] [c0000000003c9488]
do_set_mempolicy+0x108/0x170
[   16.193314][    T1] [c00000002aaefdb0] [c000000000010434]
kernel_init+0x64/0x150
[   16.193363][    T1] [c00000002aaefe20] [c00000000000b1cc]
ret_from_kernel_thread+0x5c/0x70

# ./scripts/faddr2line vmlinux log_early+0x8/0x160
log_early+0x8/0x160:
log_early at mm/kmemleak.c:859


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-31 12:02       ` Qian Cai
@ 2019-07-31 14:48         ` Catalin Marinas
  2019-07-31 14:54           ` Qian Cai
  0 siblings, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2019-07-31 14:48 UTC (permalink / raw)
  To: Qian Cai
  Cc: Andrew Morton, Linux-MM, linux-kernel, Michal Hocko, Matthew Wilcox

On Wed, Jul 31, 2019 at 08:02:30AM -0400, Qian Cai wrote:
> On Jul 31, 2019, at 5:53 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Tue, Jul 30, 2019 at 04:22:37PM -0400, Qian Cai wrote:
> >> On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
> >>> On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com>
> >>>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>>> @@ -2011,6 +2011,12 @@
> >>>>  			Built with CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
> >>>>  			the default is off.
> >>>>  
> >>>> +	kmemleak.mempool=
> >>>> +			[KNL] Boot-time tuning of the minimum kmemleak
> >>>> +			metadata pool size.
> >>>> +			Format: <int>
> >>>> +			Default: NR_CPUS * 4
> >>>> +
> >> 
> >> Catalin, BTW, it is right now unable to handle a large size. I tried to reserve
> >> 64M (kmemleak.mempool=67108864),
[...]
> > It looks like the mempool cannot be created. 64M objects means a
> > kmalloc(512MB) for the pool array in mempool_init_node(), so that hits
> > the MAX_ORDER warning in __alloc_pages_nodemask().
> > 
> > Maybe the mempool tunable won't help much for your case if you need so
> > many objects. It's still worth having a mempool for kmemleak but we
> > could look into changing the refill logic while keeping the original
> > size constant (say 1024 objects).
> 
> Actually, kmemleak.mempool=524288 works quite well on systems I have here. This
> is more of making the code robust by error-handling a large value without the
> NULL-ptr-deref below. Maybe simply just validate the value by adding upper bound
> to not trigger that warning with MAX_ORDER.

Would it work for you with a Kconfig option, similar to
DEBUG_KMEMLEAK_EARLY_LOG_SIZE?

> >> [   16.192449][    T1] BUG: Unable to handle kernel data access at 0xffffffffffffb2aa
> > 
> > This doesn't seem kmemleak related from the trace.
> 
> This only happens when passing a large kmemleak.mempool, e.g., 64M
> 
> [   16.193126][    T1] NIP [c000000000b2a2fc] log_early+0x8/0x160
> [   16.193153][    T1] LR [c0000000003e6e48] kmem_cache_free+0x428/0x740

Ah, I missed the log_early() call here. It's a kmemleak bug where it
isn't disabled properly in case of an error and log_early() is still
called after the .text.init section was freed. I'll send a patch.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-31 14:48         ` Catalin Marinas
@ 2019-07-31 14:54           ` Qian Cai
  0 siblings, 0 replies; 15+ messages in thread
From: Qian Cai @ 2019-07-31 14:54 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Andrew Morton, Linux-MM, linux-kernel, Michal Hocko, Matthew Wilcox

On Wed, 2019-07-31 at 15:48 +0100, Catalin Marinas wrote:
> On Wed, Jul 31, 2019 at 08:02:30AM -0400, Qian Cai wrote:
> > On Jul 31, 2019, at 5:53 AM, Catalin Marinas <catalin.marinas@arm.com>
> > wrote:
> > > On Tue, Jul 30, 2019 at 04:22:37PM -0400, Qian Cai wrote:
> > > > On Tue, 2019-07-30 at 12:57 -0700, Andrew Morton wrote:
> > > > > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@ar
> > > > > m.com>
> > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > > @@ -2011,6 +2011,12 @@
> > > > > >  			Built with
> > > > > > CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y,
> > > > > >  			the default is off.
> > > > > >  
> > > > > > +	kmemleak.mempool=
> > > > > > +			[KNL] Boot-time tuning of the minimum
> > > > > > kmemleak
> > > > > > +			metadata pool size.
> > > > > > +			Format: <int>
> > > > > > +			Default: NR_CPUS * 4
> > > > > > +
> > > > 
> > > > Catalin, BTW, it is right now unable to handle a large size. I tried to
> > > > reserve
> > > > 64M (kmemleak.mempool=67108864),
> 
> [...]
> > > It looks like the mempool cannot be created. 64M objects means a
> > > kmalloc(512MB) for the pool array in mempool_init_node(), so that hits
> > > the MAX_ORDER warning in __alloc_pages_nodemask().
> > > 
> > > Maybe the mempool tunable won't help much for your case if you need so
> > > many objects. It's still worth having a mempool for kmemleak but we
> > > could look into changing the refill logic while keeping the original
> > > size constant (say 1024 objects).
> > 
> > Actually, kmemleak.mempool=524288 works quite well on systems I have here.
> > This
> > is more of making the code robust by error-handling a large value without
> > the
> > NULL-ptr-deref below. Maybe simply just validate the value by adding upper
> > bound
> > to not trigger that warning with MAX_ORDER.
> 
> Would it work for you with a Kconfig option, similar to
> DEBUG_KMEMLEAK_EARLY_LOG_SIZE?

Yes, it should be fine.

> 
> > > > [   16.192449][    T1] BUG: Unable to handle kernel data access at
> > > > 0xffffffffffffb2aa
> > > 
> > > This doesn't seem kmemleak related from the trace.
> > 
> > This only happens when passing a large kmemleak.mempool, e.g., 64M
> > 
> > [   16.193126][    T1] NIP [c000000000b2a2fc] log_early+0x8/0x160
> > [   16.193153][    T1] LR [c0000000003e6e48] kmem_cache_free+0x428/0x740
> 
> Ah, I missed the log_early() call here. It's a kmemleak bug where it
> isn't disabled properly in case of an error and log_early() is still
> called after the .text.init section was freed. I'll send a patch.
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-30 20:02 ` Andrew Morton
@ 2019-07-31 15:44   ` Catalin Marinas
  2019-08-01  6:41     ` Michal Hocko
  0 siblings, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2019-07-31 15:44 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, Michal Hocko, Matthew Wilcox, Qian Cai

On Tue, Jul 30, 2019 at 01:02:15PM -0700, Andrew Morton wrote:
> On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> > Add mempool allocations for struct kmemleak_object and
> > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > under memory pressure. Additionally, mask out all the gfp flags passed
> > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > 
> > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > different minimum pool size (defaulting to NR_CPUS * 4).
> 
> btw, the checkpatch warnings are valid:
> 
> WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> #70: FILE: mm/kmemleak.c:197:
> +static int min_object_pool = NR_CPUS * 4;
> 
> WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> #71: FILE: mm/kmemleak.c:198:
> +static int min_scan_area_pool = NR_CPUS * 1;
> 
> There can be situations where NR_CPUS is much larger than
> num_possible_cpus().  Can we initialize these tunables within
> kmemleak_init()?

We could and, at least on arm64, cpu_possible_mask is already
initialised at that point. However, that's a totally made up number. I
think we would better go for a Kconfig option (defaulting to, say, 1024)
similar to the CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE and we grow it if
people report better values in the future.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-07-31 15:44   ` Catalin Marinas
@ 2019-08-01  6:41     ` Michal Hocko
  2019-08-03 10:48       ` Catalin Marinas
  0 siblings, 1 reply; 15+ messages in thread
From: Michal Hocko @ 2019-08-01  6:41 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Andrew Morton, linux-mm, linux-kernel, Matthew Wilcox, Qian Cai

On Wed 31-07-19 16:44:50, Catalin Marinas wrote:
> On Tue, Jul 30, 2019 at 01:02:15PM -0700, Andrew Morton wrote:
> > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 
> > > Add mempool allocations for struct kmemleak_object and
> > > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > > under memory pressure. Additionally, mask out all the gfp flags passed
> > > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > > 
> > > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > > different minimum pool size (defaulting to NR_CPUS * 4).
> > 
> > btw, the checkpatch warnings are valid:
> > 
> > WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> > #70: FILE: mm/kmemleak.c:197:
> > +static int min_object_pool = NR_CPUS * 4;
> > 
> > WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> > #71: FILE: mm/kmemleak.c:198:
> > +static int min_scan_area_pool = NR_CPUS * 1;
> > 
> > There can be situations where NR_CPUS is much larger than
> > num_possible_cpus().  Can we initialize these tunables within
> > kmemleak_init()?
> 
> We could and, at least on arm64, cpu_possible_mask is already
> initialised at that point. However, that's a totally made up number. I
> think we would better go for a Kconfig option (defaulting to, say, 1024)
> similar to the CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE and we grow it if
> people report better values in the future.

If you really want/need to make this configurable then the command line
parameter makes more sense - think of distribution kernel users for
example. But I am still not sure why this is really needed. The initial
size is a "made up" number of course. There is no good estimation to
make (without a crystal ball). The value might be increased based on
real life usage.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects
  2019-08-01  6:41     ` Michal Hocko
@ 2019-08-03 10:48       ` Catalin Marinas
  0 siblings, 0 replies; 15+ messages in thread
From: Catalin Marinas @ 2019-08-03 10:48 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, Matthew Wilcox, Qian Cai

On Thu, Aug 01, 2019 at 08:41:53AM +0200, Michal Hocko wrote:
> On Wed 31-07-19 16:44:50, Catalin Marinas wrote:
> > On Tue, Jul 30, 2019 at 01:02:15PM -0700, Andrew Morton wrote:
> > > On Sat, 27 Jul 2019 14:23:33 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > Add mempool allocations for struct kmemleak_object and
> > > > kmemleak_scan_area as slightly more resilient than kmem_cache_alloc()
> > > > under memory pressure. Additionally, mask out all the gfp flags passed
> > > > to kmemleak other than GFP_KERNEL|GFP_ATOMIC.
> > > > 
> > > > A boot-time tuning parameter (kmemleak.mempool) is added to allow a
> > > > different minimum pool size (defaulting to NR_CPUS * 4).
> > > 
> > > btw, the checkpatch warnings are valid:
> > > 
> > > WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> > > #70: FILE: mm/kmemleak.c:197:
> > > +static int min_object_pool = NR_CPUS * 4;
> > > 
> > > WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc
> > > #71: FILE: mm/kmemleak.c:198:
> > > +static int min_scan_area_pool = NR_CPUS * 1;
> > > 
> > > There can be situations where NR_CPUS is much larger than
> > > num_possible_cpus().  Can we initialize these tunables within
> > > kmemleak_init()?
> > 
> > We could and, at least on arm64, cpu_possible_mask is already
> > initialised at that point. However, that's a totally made up number. I
> > think we would better go for a Kconfig option (defaulting to, say, 1024)
> > similar to the CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE and we grow it if
> > people report better values in the future.
> 
> If you really want/need to make this configurable then the command line
> parameter makes more sense - think of distribution kernel users for
> example.

I doubt you'd have pre-built distribution kernels with kmemleak enabled.

> But I am still not sure why this is really needed. The initial
> size is a "made up" number of course. There is no good estimation to
> make (without a crystal ball). The value might be increased based on
> real life usage.

We had a similar situation with the early log buffer (before slab is
initialised), initially 400 which was good enough for my needs (embedded
systems) but others had entirely different requirements. A configurable
(cmdline, Kconfig) option would make it easier for people to change,
especially if coupled with a meaningful suggestion in dmesg.

Another option is to use the early log as an emergency pool after
initialisation instead of freeing it (it's currently __initdata) and
drop the mempool idea. I may give this a go, at least we only have a
single Kconfig option.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-08-03 10:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-27 13:23 [PATCH v2] mm: kmemleak: Use mempool allocations for kmemleak objects Catalin Marinas
2019-07-30 19:57 ` Andrew Morton
2019-07-30 20:22   ` Qian Cai
2019-07-30 20:39     ` Andrew Morton
2019-07-31  9:53     ` Catalin Marinas
2019-07-31 12:02       ` Qian Cai
2019-07-31 14:48         ` Catalin Marinas
2019-07-31 14:54           ` Qian Cai
2019-07-31  9:06   ` Michal Hocko
2019-07-31  9:17     ` Catalin Marinas
2019-07-30 20:02 ` Andrew Morton
2019-07-31 15:44   ` Catalin Marinas
2019-08-01  6:41     ` Michal Hocko
2019-08-03 10:48       ` Catalin Marinas
2019-07-31  9:10 ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).