introduce a default off mode for kmemleak

PATCH In review Jason Baron 2 participants 4 messages Jul 13, 2010 – Jul 14, 2010

Subsystems (3)

Memory management Library code Kmemleak

lore

Share & tools

lore

Jason Baron

author

2010-07-13 20:31 UTC #0 lore raw

PATCH 1/1 introduce a default off mode for kmemleak

Hi,

Kmemleak can be very useful in figuring out where a kernel leak is suspected, but I've run into issues it which makes it hard to enable by default - reports false positives, oom kills on low memory systems (512 MB). Although these can be addressed, there is still the memory footprint, and runtime overhead, so I think it might make sense to create mode where it is disabled by default, but can still be enabled on the command line via: kmemleak=on.

thanks,

-Jason

Introduce a new DEBUG_KMEMLEAK_DEFAULT_OFF config parameter that allows kmemleak to be disabled by default, but enabled on the command line via:
kmemleak=on. Although a reboot is required to turn it on, its still useful to not require a re-compile.

Signed-off-by Jason Baron <[email protected]>

---
 lib/Kconfig.debug |    7 +++++++
 mm/kmemleak.c     |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletions(-)

lib/Kconfig.debug +7 -0

index e722e9d..95ab402 100644
@@ -400,6 +400,13 @@ config DEBUG_KMEMLEAK_TEST
 
 	  If unsure, say N.
 
+config DEBUG_KMEMLEAK_DEFAULT_OFF
+	bool "Default kmemleak to off"
+	depends on DEBUG_KMEMLEAK
+	help
+	  Say Y here to disable kmemleak by default. It can then be enabled
+	  on the command line via kmemleak=on.
+
 config DEBUG_PREEMPT
 	bool "Debug preemptible kernel"
 	depends on DEBUG_KERNEL && PREEMPT && TRACE_IRQFLAGS_SUPPORT

mm/kmemleak.c +25 -1

index 2c0d032..1006267 100644
@@ -211,6 +211,11 @@ static signed long jiffies_scan_wait;
 static int kmemleak_stack_scan = 1;
 /* protects the memory scanning, parameters and debug/kmemleak file access */
 static DEFINE_MUTEX(scan_mutex);
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+/* setting kmemleak=on, will set this var, skipping the disable */
+static int kmemleak_skip_disable;
+#endif
+
 
 /*
  * Early object allocation/freeing logging. Kmemleak is initialized after the

@@ -1600,10 +1605,22 @@ static int kmemleak_boot_config(char *str)
 {
 	if (!str)
 		return -EINVAL;
+
 	if (strcmp(str, "off") == 0)
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+		return -EINVAL;
+#else
 		kmemleak_disable();
-	else if (strcmp(str, "on") != 0)
+#endif
+	else if (strcmp(str, "on") == 0)
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+		kmemleak_skip_disable = 1;
+#else
+		return -EINVAL;
+#endif
+	else
 		return -EINVAL;
+
 	return 0;
 }
 early_param("kmemleak", kmemleak_boot_config);

@@ -1616,6 +1633,13 @@ void __init kmemleak_init(void)
 	int i;
 	unsigned long flags;
 
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+	if (!kmemleak_skip_disable) {
+		kmemleak_disable();
+		return;
+	}
+#endif
+
 	jiffies_min_age = msecs_to_jiffies(MSECS_MIN_AGE);
 	jiffies_scan_wait = msecs_to_jiffies(SECS_SCAN_WAIT * 1000);

Signature · 1 line

1.7.1

Catalin Marinas

maintainer

2010-07-13 21:02 UTC #1 lore raw

Hi Jason,

Quoted 6 lines · Jason Baron

Introduce a new DEBUG_KMEMLEAK_DEFAULT_OFF config parameter that allows kmemleak to be disabled by default, but enabled on the command line via:
kmemleak=on. Although a reboot is required to turn it on, its still useful to not require a re-compile.
Signed-off-by Jason Baron <[email protected]>

The patch makes sense. I have a few comments below.

mm/kmemleak.c static signed long jiffies_scan_wait; · line 217

Quoted 12 lines

mm/kmemleak.c +4 -0

index 2c0d032..1006267 100644
@@ -211,6 +211,11 @@ static signed long jiffies_scan_wait;
 static int kmemleak_stack_scan = 1;
 /* protects the memory scanning, parameters and debug/kmemleak file access */
 static DEFINE_MUTEX(scan_mutex);
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+/* setting kmemleak=on, will set this var, skipping the disable */
+static int kmemleak_skip_disable;
+#endif

Can we have kmemleak_skip_disable always and remove the #ifdef?

Quoted 13 lines

mm/kmemleak.c +6 -1

@@ -1600,10 +1605,22 @@ static int kmemleak_boot_config(char *str)
 {
        if (!str)
                return -EINVAL;
+
        if (strcmp(str, "off") == 0)
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+               return -EINVAL;
+#else
                kmemleak_disable();
-       else if (strcmp(str, "on") != 0)
+#endif
+       else if (strcmp(str, "on") == 0)

mm/kmemleak.c static int kmemleak_boot_config(char *str) · line 1625

Quoted 10 lines

mm/kmemleak.c +7 -0

+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+               kmemleak_skip_disable = 1;
+#else
+               return -EINVAL;
+#endif
+       else
                return -EINVAL;
+
        return 0;
 }

Here I would remove all these #ifdef's. It doesn't really matter if you pass kmemleak=off/on on the command line and kmemleak was already disable/enabled. If we remove the #ifdef for the kmemleak_skip_disable, we can always set the variable here.

mm/kmemleak.c void __init kmemleak_init(void) · line 1641

Quoted 10 lines

mm/kmemleak.c +6 -0

@@ -1616,6 +1633,13 @@ void __init kmemleak_init(void)
        int i;
        unsigned long flags;
 
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+       if (!kmemleak_skip_disable) {
+               kmemleak_disable();
+               return;
+       }
+#endif

That's the only place where we could keep the #ifdef.

Thanks.

Signature · 1 line

Catalin

Jason Baron

author

2010-07-13 21:55 UTC #2 lore raw

Quoted 69 lines · Catalin Marinas

Hi Jason,
Quoted 6 lines · Jason Baron
Introduce a new DEBUG_KMEMLEAK_DEFAULT_OFF config parameter that allows kmemleak to be disabled by default, but enabled on the command line via:
kmemleak=on. Although a reboot is required to turn it on, its still useful to not require a re-compile.
Signed-off-by Jason Baron <[email protected]>
The patch makes sense. I have a few comments below.
mm/kmemleak.c static signed long jiffies_scan_wait; · line 217
Quoted 12 lines
mm/kmemleak.c +4 -0
index 2c0d032..1006267 100644
@@ -211,6 +211,11 @@ static signed long jiffies_scan_wait;
 static int kmemleak_stack_scan = 1;
 /* protects the memory scanning, parameters and debug/kmemleak file access */
 static DEFINE_MUTEX(scan_mutex);
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+/* setting kmemleak=on, will set this var, skipping the disable */
+static int kmemleak_skip_disable;
+#endif
Can we have kmemleak_skip_disable always and remove the #ifdef?
Quoted 13 lines
mm/kmemleak.c +6 -1
@@ -1600,10 +1605,22 @@ static int kmemleak_boot_config(char *str)
 {
        if (!str)
                return -EINVAL;
+
        if (strcmp(str, "off") == 0)
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+               return -EINVAL;
+#else
                kmemleak_disable();
-       else if (strcmp(str, "on") != 0)
+#endif
+       else if (strcmp(str, "on") == 0)
mm/kmemleak.c static int kmemleak_boot_config(char *str) · line 1625
Quoted 10 lines
mm/kmemleak.c +7 -0
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+               kmemleak_skip_disable = 1;
+#else
+               return -EINVAL;
+#endif
+       else
                return -EINVAL;
+
        return 0;
 }
Here I would remove all these #ifdef's. It doesn't really matter if you pass kmemleak=off/on on the command line and kmemleak was already disable/enabled. If we remove the #ifdef for the kmemleak_skip_disable, we can always set the variable here.
mm/kmemleak.c void __init kmemleak_init(void) · line 1641
Quoted 10 lines
mm/kmemleak.c +6 -0
@@ -1616,6 +1633,13 @@ void __init kmemleak_init(void)
        int i;
        unsigned long flags;
 
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+       if (!kmemleak_skip_disable) {
+               kmemleak_disable();
+               return;
+       }
+#endif
That's the only place where we could keep the #ifdef.

Quoted 5 lines

Thanks.
Signature · 1 line
Catalin

Ok. re-vised patch is below.

thanks,

-Jason

Introduce a new DEBUG_KMEMLEAK_DEFAULT_OFF config parameter that allows kmemleak to be disabled by default, but enabled on the command line
via: kmemleak=on. Although a reboot is required to turn it on, its still useful to not require a re-compile.

Signed-off-by Jason Baron <[email protected]>

---
 lib/Kconfig.debug |    7 +++++++
 mm/kmemleak.c     |   14 +++++++++++++-
 2 files changed, 20 insertions(+), 1 deletions(-)

lib/Kconfig.debug +7 -0

index e722e9d..95ab402 100644
@@ -400,6 +400,13 @@ config DEBUG_KMEMLEAK_TEST
 
 	  If unsure, say N.
 
+config DEBUG_KMEMLEAK_DEFAULT_OFF
+	bool "Default kmemleak to off"
+	depends on DEBUG_KMEMLEAK
+	help
+	  Say Y here to disable kmemleak by default. It can then be enabled
+	  on the command line via kmemleak=on.
+
 config DEBUG_PREEMPT
 	bool "Debug preemptible kernel"
 	depends on DEBUG_KERNEL && PREEMPT && TRACE_IRQFLAGS_SUPPORT

mm/kmemleak.c +13 -1

index 2c0d032..c7ff733 100644
@@ -211,6 +211,9 @@ static signed long jiffies_scan_wait;
 static int kmemleak_stack_scan = 1;
 /* protects the memory scanning, parameters and debug/kmemleak file access */
 static DEFINE_MUTEX(scan_mutex);
+/* setting kmemleak=on, will set this var, skipping the disable */
+static int kmemleak_skip_disable;
+
 
 /*
  * Early object allocation/freeing logging. Kmemleak is initialized after the

@@ -1602,7 +1605,9 @@ static int kmemleak_boot_config(char *str)
 		return -EINVAL;
 	if (strcmp(str, "off") == 0)
 		kmemleak_disable();
-	else if (strcmp(str, "on") != 0)
+	else if (strcmp(str, "on") == 0)
+		kmemleak_skip_disable = 1;
+	else
 		return -EINVAL;
 	return 0;
 }

@@ -1616,6 +1621,13 @@ void __init kmemleak_init(void)
 	int i;
 	unsigned long flags;
 
+#ifdef CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
+	if (!kmemleak_skip_disable) {
+		kmemleak_disable();
+		return;
+	}
+#endif
+
 	jiffies_min_age = msecs_to_jiffies(MSECS_MIN_AGE);
 	jiffies_scan_wait = msecs_to_jiffies(SECS_SCAN_WAIT * 1000);

Signature · 1 line

1.7.1

Catalin Marinas

maintainer

2010-07-14 09:42 UTC #3 lore raw

Quoted 6 lines · Jason Baron

Introduce a new DEBUG_KMEMLEAK_DEFAULT_OFF config parameter that allows kmemleak to be disabled by default, but enabled on the command line
via: kmemleak=on. Although a reboot is required to turn it on, its still useful to not require a re-compile.
Signed-off-by Jason Baron <[email protected]>

Thanks, it looks fine to me. I'll merge it in my tree and send Linus a pull request during the merging window (unless you prefer to push it yourself, in which case I ack it).

Signature · 1 line

Catalin