Prev: Build log scripts
Next: [tip:x86/setup] x86, setup: Make the setup code also accept console=uart8250
From: Catalin Marinas on 13 Jul 2010 17:10
Hi Jason,

On Tue, 2010-07-13 at 21:31 +0100, Jason Baron wrote:
> 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 <jbaron(a)redhat.com>

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

> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 2c0d032..1006267 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -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?

> @@ -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;
> }

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.

> @@ -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.

--
Catalin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jason Baron on 13 Jul 2010 18:00
On Tue, Jul 13, 2010 at 10:02:12PM +0100, Catalin Marinas wrote:
> Hi Jason,
>
> On Tue, 2010-07-13 at 21:31 +0100, Jason Baron wrote:
> > 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 <jbaron(a)redhat.com>
>
> The patch makes sense. I have a few comments below.
>
> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > index 2c0d032..1006267 100644
> > --- a/mm/kmemleak.c
> > +++ b/mm/kmemleak.c
> > @@ -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?
>
> > @@ -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;
> > }
>
> 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.
>
> > @@ -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.
>
> --
> 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 <jbaron(a)redhat.com>
---
lib/Kconfig.debug | 7 +++++++
mm/kmemleak.c | 14 +++++++++++++-
2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e722e9d..95ab402 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 2c0d032..c7ff733 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -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);

--
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Catalin Marinas on 14 Jul 2010 05:50
On Tue, 2010-07-13 at 22:55 +0100, Jason Baron wrote:
> 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 <jbaron(a)redhat.com>

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).

--
Catalin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Build log scripts
Next: [tip:x86/setup] x86, setup: Make the setup code also accept console=uart8250