[PATCH] [v2] TAINT_PERFORMANCE

[PATCH] [v2] TAINT_PERFORMANCE

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Changes from v1:
 * remove schedstats
 * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON

--

I have more than once myself been the victim of an accidentally-
enabled kernel config option being mistaken for a true
performance problem.

I'm sure I've also taken profiles or performance measurements
and assumed they were real-world when really I was measuing the
performance with an option that nobody turns on in production.

A warning like this late in boot will help remind folks when
these kinds of things are enabled.

As for the patch...

I originally wanted this for CONFIG_DEBUG_VM, but I think it also
applies to things like lockdep and slab debugging.  See the patch
for the list of offending config options.  I'm open to adding
more, but this seemed like a good list to start.

This could be done with Kconfig and an #ifdef to save us 8 bytes
of text and the entry in the late_initcall() section.  Doing it
this way lets us keep the list of these things in one spot, and
also gives us a convenient way to dump out the name of the
offending option.

The dump_stack() is really just to be loud.

For anybody that *really* cares, I put the whole thing under
#ifdef CONFIG_DEBUG_KERNEL.

The messages look like this:

[    2.534574] CONFIG_LOCKDEP enabled
[    2.536392] Do not use this kernel for performance measurement.
[    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
[    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
[    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
[    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
[    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
[    2.592638] Call Trace:
[    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68
[    2.597742]  [<ffffffff81dca5b6>] ? oops_setup+0x2e/0x2e
[    2.601247]  [<ffffffff81dca5e2>] performance_taint+0x2c/0x3c
[    2.603498]  [<ffffffff81000377>] do_one_initcall+0xe7/0x290
[    2.606556]  [<ffffffff81db3215>] kernel_init_freeable+0x106/0x19a
[    2.609718]  [<ffffffff81db29e8>] ? do_early_param+0x86/0x86
[    2.613772]  [<ffffffff817bcfc0>] ? rest_init+0x150/0x150
[    2.617333]  [<ffffffff817bcfce>] kernel_init+0xe/0xf0
[    2.620840]  [<ffffffff817dbc7c>] ret_from_fork+0x7c/0xb0
[    2.624718]  [<ffffffff817bcfc0>] ? rest_init+0x150/0x150

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: ak@linux.intel.com
Cc: tim.c.chen@linux.intel.com
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: kirill@shutemov.name
Cc: lauraa@codeaurora.org
---

 b/include/linux/kernel.h |    1 +
 b/kernel/panic.c         |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff -puN include/linux/kernel.h~taint-performance include/linux/kernel.h
--- a/include/linux/kernel.h~taint-performance	2014-08-19 11:38:07.424005355 -0700
+++ b/include/linux/kernel.h	2014-08-19 11:38:20.960615904 -0700
@@ -471,6 +471,7 @@ extern enum system_states {
 #define TAINT_OOT_MODULE		12
 #define TAINT_UNSIGNED_MODULE		13
 #define TAINT_SOFTLOCKUP		14
+#define TAINT_PERFORMANCE		15
 
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
diff -puN kernel/panic.c~taint-performance kernel/panic.c
--- a/kernel/panic.c~taint-performance	2014-08-19 11:38:28.928975233 -0700
+++ b/kernel/panic.c	2014-08-19 20:56:31.598705180 -0700
@@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
 	{ TAINT_OOT_MODULE,		'O', ' ' },
 	{ TAINT_UNSIGNED_MODULE,	'E', ' ' },
 	{ TAINT_SOFTLOCKUP,		'L', ' ' },
+	{ TAINT_PERFORMANCE,		'Q', ' ' },
 };
 
 /**
@@ -501,3 +502,43 @@ static int __init oops_setup(char *s)
 	return 0;
 }
 early_param("oops", oops_setup);
+
+#ifdef CONFIG_DEBUG_KERNEL
+#define TAINT_PERF_IF(x) do {						\
+		if (IS_ENABLED(CONFIG_##x)) {				\
+			do_taint = 1;					\
+			pr_warn("CONFIG_%s enabled\n",	__stringify(x));\
+		}							\
+	} while (0)
+
+static int __init performance_taint(void)
+{
+	int do_taint = 0;
+
+	/*
+	 * This should list any kernel options that can substantially
+	 * affect performance.  This is intended to give a big, fat
+	 * warning during bootup so that folks have a fighting chance
+	 * of noticing these things.
+	 */
+	TAINT_PERF_IF(LOCKDEP);
+	TAINT_PERF_IF(LOCK_STAT);
+	TAINT_PERF_IF(DEBUG_VM);
+	TAINT_PERF_IF(DEBUG_VM_VMACACHE);
+	TAINT_PERF_IF(DEBUG_VM_RB);
+	TAINT_PERF_IF(DEBUG_SLAB);
+	TAINT_PERF_IF(DEBUG_OBJECTS_FREE);
+	TAINT_PERF_IF(DEBUG_KMEMLEAK);
+	TAINT_PERF_IF(DEBUG_PAGEALLOC);
+	TAINT_PERF_IF(SLUB_DEBUG_ON);
+
+	if (!do_taint)
+		return 0;
+
+	pr_warn("Do not use this kernel for performance measurement.");
+	dump_stack();
+	add_taint(TAINT_PERFORMANCE, LOCKDEP_STILL_OK);
+	return 0;
+}
+late_initcall(performance_taint);
+#endif
_

[PATCH] [v2] TAINT_PERFORMANCE

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Changes from v1:
 * remove schedstats
 * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON

--

I have more than once myself been the victim of an accidentally-
enabled kernel config option being mistaken for a true
performance problem.

I'm sure I've also taken profiles or performance measurements
and assumed they were real-world when really I was measuing the
performance with an option that nobody turns on in production.

A warning like this late in boot will help remind folks when
these kinds of things are enabled.

As for the patch...

I originally wanted this for CONFIG_DEBUG_VM, but I think it also
applies to things like lockdep and slab debugging.  See the patch
for the list of offending config options.  I'm open to adding
more, but this seemed like a good list to start.

This could be done with Kconfig and an #ifdef to save us 8 bytes
of text and the entry in the late_initcall() section.  Doing it
this way lets us keep the list of these things in one spot, and
also gives us a convenient way to dump out the name of the
offending option.

The dump_stack() is really just to be loud.

For anybody that *really* cares, I put the whole thing under
#ifdef CONFIG_DEBUG_KERNEL.

The messages look like this:

[    2.534574] CONFIG_LOCKDEP enabled
[    2.536392] Do not use this kernel for performance measurement.
[    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
[    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
[    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
[    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
[    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
[    2.592638] Call Trace:
[    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68
[    2.597742]  [<ffffffff81dca5b6>] ? oops_setup+0x2e/0x2e
[    2.601247]  [<ffffffff81dca5e2>] performance_taint+0x2c/0x3c
[    2.603498]  [<ffffffff81000377>] do_one_initcall+0xe7/0x290
[    2.606556]  [<ffffffff81db3215>] kernel_init_freeable+0x106/0x19a
[    2.609718]  [<ffffffff81db29e8>] ? do_early_param+0x86/0x86
[    2.613772]  [<ffffffff817bcfc0>] ? rest_init+0x150/0x150
[    2.617333]  [<ffffffff817bcfce>] kernel_init+0xe/0xf0
[    2.620840]  [<ffffffff817dbc7c>] ret_from_fork+0x7c/0xb0
[    2.624718]  [<ffffffff817bcfc0>] ? rest_init+0x150/0x150

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: ak@linux.intel.com
Cc: tim.c.chen@linux.intel.com
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: kirill@shutemov.name
Cc: lauraa@codeaurora.org
---

 b/include/linux/kernel.h |    1 +
 b/kernel/panic.c         |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff -puN include/linux/kernel.h~taint-performance include/linux/kernel.h
--- a/include/linux/kernel.h~taint-performance	2014-08-19 11:38:07.424005355 -0700
+++ b/include/linux/kernel.h	2014-08-19 11:38:20.960615904 -0700
@@ -471,6 +471,7 @@ extern enum system_states {
 #define TAINT_OOT_MODULE		12
 #define TAINT_UNSIGNED_MODULE		13
 #define TAINT_SOFTLOCKUP		14
+#define TAINT_PERFORMANCE		15
 
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
diff -puN kernel/panic.c~taint-performance kernel/panic.c
--- a/kernel/panic.c~taint-performance	2014-08-19 11:38:28.928975233 -0700
+++ b/kernel/panic.c	2014-08-19 20:56:31.598705180 -0700
@@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
 	{ TAINT_OOT_MODULE,		'O', ' ' },
 	{ TAINT_UNSIGNED_MODULE,	'E', ' ' },
 	{ TAINT_SOFTLOCKUP,		'L', ' ' },
+	{ TAINT_PERFORMANCE,		'Q', ' ' },
 };
 
 /**
@@ -501,3 +502,43 @@ static int __init oops_setup(char *s)
 	return 0;
 }
 early_param("oops", oops_setup);
+
+#ifdef CONFIG_DEBUG_KERNEL
+#define TAINT_PERF_IF(x) do {						\
+		if (IS_ENABLED(CONFIG_##x)) {				\
+			do_taint = 1;					\
+			pr_warn("CONFIG_%s enabled\n",	__stringify(x));\
+		}							\
+	} while (0)
+
+static int __init performance_taint(void)
+{
+	int do_taint = 0;
+
+	/*
+	 * This should list any kernel options that can substantially
+	 * affect performance.  This is intended to give a big, fat
+	 * warning during bootup so that folks have a fighting chance
+	 * of noticing these things.
+	 */
+	TAINT_PERF_IF(LOCKDEP);
+	TAINT_PERF_IF(LOCK_STAT);
+	TAINT_PERF_IF(DEBUG_VM);
+	TAINT_PERF_IF(DEBUG_VM_VMACACHE);
+	TAINT_PERF_IF(DEBUG_VM_RB);
+	TAINT_PERF_IF(DEBUG_SLAB);
+	TAINT_PERF_IF(DEBUG_OBJECTS_FREE);
+	TAINT_PERF_IF(DEBUG_KMEMLEAK);
+	TAINT_PERF_IF(DEBUG_PAGEALLOC);
+	TAINT_PERF_IF(SLUB_DEBUG_ON);
+
+	if (!do_taint)
+		return 0;
+
+	pr_warn("Do not use this kernel for performance measurement.");
+	dump_stack();
+	add_taint(TAINT_PERFORMANCE, LOCKDEP_STILL_OK);
+	return 0;
+}
+late_initcall(performance_taint);
+#endif
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Changes from v1:
>  * remove schedstats
>  * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON
> 
> --
> 
> I have more than once myself been the victim of an accidentally-
> enabled kernel config option being mistaken for a true
> performance problem.
> 
> I'm sure I've also taken profiles or performance measurements
> and assumed they were real-world when really I was measuing the
> performance with an option that nobody turns on in production.

Most of these options already announce themselves in the 
syslog.

> A warning like this late in boot will help remind folks when
> these kinds of things are enabled.
> 
> As for the patch...
> 
> I originally wanted this for CONFIG_DEBUG_VM, but I think it also
> applies to things like lockdep and slab debugging.  See the patch
> for the list of offending config options.  I'm open to adding
> more, but this seemed like a good list to start.

> [    2.534574] CONFIG_LOCKDEP enabled
> [    2.536392] Do not use this kernel for performance measurement.

This is workload dependent: for many kernel workloads this is 
indeed true. For many user-space workloads it will add very 
little overhead.

> [    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
> [    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> [    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
> [    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
> [    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
> [    2.592638] Call Trace:
> [    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68

Generating a stack dump that tells us nothing isn't really 
useful.

>  	{ TAINT_SOFTLOCKUP,		'L', ' ' },
> +	{ TAINT_PERFORMANCE,		'Q', ' ' },

Also this looks like a slight abuse of the taint flag: we taint 
the kernel if there's a problem with it. But even for many 
types of performance measurements, a debug kernel is just fine. 
For other types of performance measurements, even a non-debug 
kernel option can have big impact.

A better option might be to declare known performance killers 
in /proc/config_debug or so, and maybe print them once at the 
end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That 
way tooling (benchmarks, profilers, etc.) can print them, but 
it's also present in the syslog, just in case.

/proc/config_debug is different from /proc/config.gz IKCONFIG, 
because it would always be present when performance impacting 
options are enabled. So tools would only have to check the 
existence of this file, for the simplest test.

In any case I don't think it's a good idea to abuse existing 
facilities just to gain attention: you'll get the extra 
attention, but the abuse dillutes the utility of those only 
tangentially related facilities.

Thanks,

	Ingo

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Changes from v1:
>  * remove schedstats
>  * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON
> 
> --
> 
> I have more than once myself been the victim of an accidentally-
> enabled kernel config option being mistaken for a true
> performance problem.
> 
> I'm sure I've also taken profiles or performance measurements
> and assumed they were real-world when really I was measuing the
> performance with an option that nobody turns on in production.

Most of these options already announce themselves in the 
syslog.

> A warning like this late in boot will help remind folks when
> these kinds of things are enabled.
> 
> As for the patch...
> 
> I originally wanted this for CONFIG_DEBUG_VM, but I think it also
> applies to things like lockdep and slab debugging.  See the patch
> for the list of offending config options.  I'm open to adding
> more, but this seemed like a good list to start.

> [    2.534574] CONFIG_LOCKDEP enabled
> [    2.536392] Do not use this kernel for performance measurement.

This is workload dependent: for many kernel workloads this is 
indeed true. For many user-space workloads it will add very 
little overhead.

> [    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
> [    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> [    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
> [    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
> [    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
> [    2.592638] Call Trace:
> [    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68

Generating a stack dump that tells us nothing isn't really 
useful.

>  	{ TAINT_SOFTLOCKUP,		'L', ' ' },
> +	{ TAINT_PERFORMANCE,		'Q', ' ' },

Also this looks like a slight abuse of the taint flag: we taint 
the kernel if there's a problem with it. But even for many 
types of performance measurements, a debug kernel is just fine. 
For other types of performance measurements, even a non-debug 
kernel option can have big impact.

A better option might be to declare known performance killers 
in /proc/config_debug or so, and maybe print them once at the 
end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That 
way tooling (benchmarks, profilers, etc.) can print them, but 
it's also present in the syslog, just in case.

/proc/config_debug is different from /proc/config.gz IKCONFIG, 
because it would always be present when performance impacting 
options are enabled. So tools would only have to check the 
existence of this file, for the simplest test.

In any case I don't think it's a good idea to abuse existing 
facilities just to gain attention: you'll get the extra 
attention, but the abuse dillutes the utility of those only 
tangentially related facilities.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Josh Boyer]

On Wed, Aug 20, 2014 at 4:11 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dave Hansen <dave@sr71.net> wrote:
>
>>
>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>
>> Changes from v1:
>>  * remove schedstats
>>  * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON
>>
>> --
>>
>> I have more than once myself been the victim of an accidentally-
>> enabled kernel config option being mistaken for a true
>> performance problem.
>>
>> I'm sure I've also taken profiles or performance measurements
>> and assumed they were real-world when really I was measuing the
>> performance with an option that nobody turns on in production.
>
> Most of these options already announce themselves in the
> syslog.
>
>> A warning like this late in boot will help remind folks when
>> these kinds of things are enabled.
>>
>> As for the patch...
>>
>> I originally wanted this for CONFIG_DEBUG_VM, but I think it also
>> applies to things like lockdep and slab debugging.  See the patch
>> for the list of offending config options.  I'm open to adding
>> more, but this seemed like a good list to start.
>
>> [    2.534574] CONFIG_LOCKDEP enabled
>> [    2.536392] Do not use this kernel for performance measurement.
>
> This is workload dependent: for many kernel workloads this is
> indeed true. For many user-space workloads it will add very
> little overhead.
>
>> [    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
>> [    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> [    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
>> [    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
>> [    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
>> [    2.592638] Call Trace:
>> [    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68
>
> Generating a stack dump that tells us nothing isn't really
> useful.

It will also immediately cause any software like ABRT to file
pointless bugs on kernels built with those options.  Please don't do
this.

>>       { TAINT_SOFTLOCKUP,             'L', ' ' },
>> +     { TAINT_PERFORMANCE,            'Q', ' ' },
>
> Also this looks like a slight abuse of the taint flag: we taint
> the kernel if there's a problem with it. But even for many
> types of performance measurements, a debug kernel is just fine.
> For other types of performance measurements, even a non-debug
> kernel option can have big impact.
>
> A better option might be to declare known performance killers
> in /proc/config_debug or so, and maybe print them once at the
> end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That
> way tooling (benchmarks, profilers, etc.) can print them, but
> it's also present in the syslog, just in case.
>
> /proc/config_debug is different from /proc/config.gz IKCONFIG,
> because it would always be present when performance impacting
> options are enabled. So tools would only have to check the
> existence of this file, for the simplest test.
>
> In any case I don't think it's a good idea to abuse existing
> facilities just to gain attention: you'll get the extra
> attention, but the abuse dillutes the utility of those only
> tangentially related facilities.

I agree.

josh

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Josh Boyer]

On Wed, Aug 20, 2014 at 4:11 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dave Hansen <dave@sr71.net> wrote:
>
>>
>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>
>> Changes from v1:
>>  * remove schedstats
>>  * add DEBUG_PAGEALLOC and SLUB_DEBUG_ON
>>
>> --
>>
>> I have more than once myself been the victim of an accidentally-
>> enabled kernel config option being mistaken for a true
>> performance problem.
>>
>> I'm sure I've also taken profiles or performance measurements
>> and assumed they were real-world when really I was measuing the
>> performance with an option that nobody turns on in production.
>
> Most of these options already announce themselves in the
> syslog.
>
>> A warning like this late in boot will help remind folks when
>> these kinds of things are enabled.
>>
>> As for the patch...
>>
>> I originally wanted this for CONFIG_DEBUG_VM, but I think it also
>> applies to things like lockdep and slab debugging.  See the patch
>> for the list of offending config options.  I'm open to adding
>> more, but this seemed like a good list to start.
>
>> [    2.534574] CONFIG_LOCKDEP enabled
>> [    2.536392] Do not use this kernel for performance measurement.
>
> This is workload dependent: for many kernel workloads this is
> indeed true. For many user-space workloads it will add very
> little overhead.
>
>> [    2.547189] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-10473-gc8d6637-dirty #800
>> [    2.558075] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> [    2.564483]  0000000080000000 ffff88009c70be78 ffffffff817ce318 0000000000000000
>> [    2.582505]  ffffffff81dca5b6 ffff88009c70be88 ffffffff81dca5e2 ffff88009c70bef8
>> [    2.588589]  ffffffff81000377 0000000000000000 0007000700000142 ffffffff81b78968
>> [    2.592638] Call Trace:
>> [    2.593762]  [<ffffffff817ce318>] dump_stack+0x4e/0x68
>
> Generating a stack dump that tells us nothing isn't really
> useful.

It will also immediately cause any software like ABRT to file
pointless bugs on kernels built with those options.  Please don't do
this.

>>       { TAINT_SOFTLOCKUP,             'L', ' ' },
>> +     { TAINT_PERFORMANCE,            'Q', ' ' },
>
> Also this looks like a slight abuse of the taint flag: we taint
> the kernel if there's a problem with it. But even for many
> types of performance measurements, a debug kernel is just fine.
> For other types of performance measurements, even a non-debug
> kernel option can have big impact.
>
> A better option might be to declare known performance killers
> in /proc/config_debug or so, and maybe print them once at the
> end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That
> way tooling (benchmarks, profilers, etc.) can print them, but
> it's also present in the syslog, just in case.
>
> /proc/config_debug is different from /proc/config.gz IKCONFIG,
> because it would always be present when performance impacting
> options are enabled. So tools would only have to check the
> existence of this file, for the simplest test.
>
> In any case I don't think it's a good idea to abuse existing
> facilities just to gain attention: you'll get the extra
> attention, but the abuse dillutes the utility of those only
> tangentially related facilities.

I agree.

josh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Dave Hansen]

On 08/20/2014 01:11 AM, Ingo Molnar wrote:
> In any case I don't think it's a good idea to abuse existing 
> facilities just to gain attention: you'll get the extra 
> attention, but the abuse dilutes the utility of those only 
> tangentially related facilities.

I'm happy to rip the TAINT parts out.  I was just hoping that some
tooling might pick up the taint flags today, and this could get picked
up without modification of whatever those tools are.

I was _really_ hoping the dmesg from the taint would be ugly and loud
enough to be sufficient, but it was relatively terse.

> A better option might be to declare known performance killers 
> in /proc/config_debug or so, and maybe print them once at the 
> end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That 
> way tooling (benchmarks, profilers, etc.) can print them, but 
> it's also present in the syslog, just in case.

Sounds reasonable to me.  As long as we have _something_ that shows up
in dmesg, it will help.

Re: [PATCH] [v2] TAINT_PERFORMANCE

[Dave Hansen]

On 08/20/2014 01:11 AM, Ingo Molnar wrote:
> In any case I don't think it's a good idea to abuse existing 
> facilities just to gain attention: you'll get the extra 
> attention, but the abuse dilutes the utility of those only 
> tangentially related facilities.

I'm happy to rip the TAINT parts out.  I was just hoping that some
tooling might pick up the taint flags today, and this could get picked
up without modification of whatever those tools are.

I was _really_ hoping the dmesg from the taint would be ugly and loud
enough to be sufficient, but it was relatively terse.

> A better option might be to declare known performance killers 
> in /proc/config_debug or so, and maybe print them once at the 
> end of the bootup, with a 'WARNING:' or 'INFO:' prefix. That 
> way tooling (benchmarks, profilers, etc.) can print them, but 
> it's also present in the syslog, just in case.

Sounds reasonable to me.  As long as we have _something_ that shows up
in dmesg, it will help.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>