* [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
@ 2014-08-21 20:24 Dave Hansen
2014-08-21 20:57 ` Dave Jones
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Dave Hansen @ 2014-08-21 20:24 UTC (permalink / raw
To: linux-kernel
Cc: Dave Hansen, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm, cl,
penberg, linux-mm, kirill, lauraa
From: Dave Hansen <dave.hansen@linux.intel.com>
Changes from v2:
* remove tainting and stack track
* add debugfs file
* added a little text to guide folks who want to add more
options
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. We can also teach tooling to
look for and capture /sys/kernel/debug/config_debug .
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.
The compiler is smart enough to really trim down the code when
the array is empty. An objdump -d looks like this:
lib/perf-configs.o: file format elf64-x86-64
Disassembly of section .init.text:
0000000000000000 <performance_taint>:
0: 55 push %rbp
1: 31 c0 xor %eax,%eax
3: 48 89 e5 mov %rsp,%rbp
6: 5d pop %rbp
7: c3 retq
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.
For anybody that *really* cares, I put the whole thing under
CONFIG_DEBUG_KERNEL in the Makefile.
The messages look like this:
[ 3.865297] WARNING: Do not use this kernel for performance measurement.
[ 3.868776] WARNING: Potentially performance-altering options:
[ 3.871558] CONFIG_LOCKDEP enabled
[ 3.873326] CONFIG_SLUB_DEBUG_ON enabled
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 | 1
b/lib/Makefile | 1
b/lib/perf-configs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 117 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-20 09:56:29.528471033 -0700
@@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
{ TAINT_OOT_MODULE, 'O', ' ' },
{ TAINT_UNSIGNED_MODULE, 'E', ' ' },
{ TAINT_SOFTLOCKUP, 'L', ' ' },
+ { TAINT_PERFORMANCE, 'Q', ' ' },
};
/**
diff -puN /dev/null lib/perf-configs.c
--- /dev/null 2014-04-10 11:28:14.066815724 -0700
+++ b/lib/perf-configs.c 2014-08-21 13:22:25.586598278 -0700
@@ -0,0 +1,114 @@
+#include <linux/bug.h>
+#include <linux/debugfs.h>
+#include <linux/gfp.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+/*
+ * This should list any kernel options that can substantially
+ * affect performance. This is intended to give a loud
+ * warning during bootup so that folks have a fighting chance
+ * of noticing these things.
+ *
+ * This is fairly subjective, but a good rule of thumb for these
+ * is: if it is enabled widely in production, then it does not
+ * belong here. If a major enterprise kernel enables a feature
+ * for a non-debug kernel, it _really_ does not belong.
+ */
+static const char * const perfomance_killing_configs[] = {
+#ifdef CONFIG_LOCKDEP
+ "LOCKDEP",
+#endif
+#ifdef CONFIG_LOCK_STAT
+ "LOCK_STAT",
+#endif
+#ifdef CONFIG_DEBUG_VM
+ "DEBUG_VM",
+#endif
+#ifdef CONFIG_DEBUG_VM_VMACACHE
+ "DEBUG_VM_VMACACHE",
+#endif
+#ifdef CONFIG_DEBUG_VM_RB
+ "DEBUG_VM_RB",
+#endif
+#ifdef CONFIG_DEBUG_SLAB
+ "DEBUG_SLAB",
+#endif
+#ifdef CONFIG_SLUB_DEBUG_ON
+ "SLUB_DEBUG_ON",
+#endif
+#ifdef CONFIG_DEBUG_OBJECTS_FREE
+ "DEBUG_OBJECTS_FREE",
+#endif
+#ifdef CONFIG_DEBUG_KMEMLEAK
+ "DEBUG_KMEMLEAK",
+#endif
+#ifdef CONFIG_DEBUG_PAGEALLOC
+ "DEBUG_PAGEALLOC",
+#endif
+};
+
+static const char config_prefix[] = "CONFIG_";
+/*
+ * Dump out the list of the offending config options to a file
+ * in debugfs so that tooling can look for and capture it.
+ */
+static ssize_t performance_taint_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ int i;
+ int ret;
+ char *buf;
+ size_t buf_written = 0;
+ size_t buf_left;
+ size_t buf_len;
+
+ if (!ARRAY_SIZE(perfomance_killing_configs))
+ return 0;
+
+ buf_len = 1;
+ for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++)
+ buf_len += strlen(config_prefix) +
+ strlen(perfomance_killing_configs[i]);
+ /* Add a byte for for each entry in the array for a \n */
+ buf_len += ARRAY_SIZE(perfomance_killing_configs);
+
+ buf = kmalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ buf_left = buf_len;
+ for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
+ buf_written += snprintf(buf + buf_written, buf_left,
+ "%s%s\n", config_prefix,
+ perfomance_killing_configs[i]);
+ buf_left = buf_len - buf_written;
+ }
+ ret = simple_read_from_buffer(user_buf, buf_written, ppos, buf, buf_len);
+ kfree(buf);
+ return ret;
+}
+
+static const struct file_operations fops_perf_taint = {
+ .read = performance_taint_read,
+ .llseek = default_llseek,
+};
+
+static int __init performance_taint(void)
+{
+ int i;
+
+ if (!ARRAY_SIZE(perfomance_killing_configs))
+ return 0;
+
+ pr_warn("WARNING: Do not use this kernel for performance measurement.\n");
+ pr_warn("WARNING: Potentially performance-altering options:\n");
+ for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
+ pr_warn("\t%s%s enabled\n", config_prefix,
+ perfomance_killing_configs[i]);
+ }
+ debugfs_create_file("config_debug", S_IRUSR | S_IWUSR,
+ NULL, NULL, &fops_perf_taint);
+ return 0;
+}
+late_initcall(performance_taint);
diff -puN lib/Makefile~taint-performance lib/Makefile
--- a/lib/Makefile~taint-performance 2014-08-20 11:02:54.130548350 -0700
+++ b/lib/Makefile 2014-08-20 11:06:18.231744868 -0700
@@ -54,6 +54,7 @@ obj-$(CONFIG_GENERIC_HWEIGHT) += hweight
obj-$(CONFIG_BTREE) += btree.o
obj-$(CONFIG_INTERVAL_TREE) += interval_tree.o
obj-$(CONFIG_ASSOCIATIVE_ARRAY) += assoc_array.o
+obj-$(CONFIG_DEBUG_KERNEL) += perf-configs.o
obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
obj-$(CONFIG_DEBUG_LIST) += list_debug.o
obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
_
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-21 20:24 [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE Dave Hansen
@ 2014-08-21 20:57 ` Dave Jones
2014-08-21 21:03 ` Dave Hansen
2014-08-22 7:20 ` Ingo Molnar
2014-08-22 16:32 ` Tim Chen
2 siblings, 1 reply; 10+ messages in thread
From: Dave Jones @ 2014-08-21 20:57 UTC (permalink / raw
To: Dave Hansen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm,
cl, penberg, linux-mm, kirill, lauraa
On Thu, Aug 21, 2014 at 01:24:24PM -0700, Dave Hansen wrote:
> Changes from v2:
> * remove tainting and stack track
...
> 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-20 09:56:29.528471033 -0700
> @@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
> { TAINT_OOT_MODULE, 'O', ' ' },
> { TAINT_UNSIGNED_MODULE, 'E', ' ' },
> { TAINT_SOFTLOCKUP, 'L', ' ' },
> + { TAINT_PERFORMANCE, 'Q', ' ' },
You don't need these any more.
Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-21 20:57 ` Dave Jones
@ 2014-08-21 21:03 ` Dave Hansen
0 siblings, 0 replies; 10+ messages in thread
From: Dave Hansen @ 2014-08-21 21:03 UTC (permalink / raw
To: Dave Jones, linux-kernel, dave.hansen, peterz, mingo, ak,
tim.c.chen, akpm, cl, penberg, linux-mm, kirill, lauraa
On 08/21/2014 01:57 PM, Dave Jones wrote:
> > 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-20 09:56:29.528471033 -0700
> > @@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
> > { TAINT_OOT_MODULE, 'O', ' ' },
> > { TAINT_UNSIGNED_MODULE, 'E', ' ' },
> > { TAINT_SOFTLOCKUP, 'L', ' ' },
> > + { TAINT_PERFORMANCE, 'Q', ' ' },
>
> You don't need these any more.
Bah, thanks for catching that. I'll wait a bit for any other comments
and send out a fixed version.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-21 20:24 [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE Dave Hansen
2014-08-21 20:57 ` Dave Jones
@ 2014-08-22 7:20 ` Ingo Molnar
2014-08-22 15:02 ` Dave Hansen
2014-08-22 16:32 ` Tim Chen
2 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2014-08-22 7:20 UTC (permalink / raw
To: Dave Hansen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm,
cl, penberg, linux-mm, kirill, lauraa, Linus Torvalds,
Peter Zijlstra, Thomas Gleixner
* Dave Hansen <dave@sr71.net> wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> Changes from v2:
> * remove tainting and stack track
> * add debugfs file
> * added a little text to guide folks who want to add more
> options
Looks good to me conceptually.
A couple of minor details:
> 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
s/measuring
> 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. We can also teach tooling to
> look for and capture /sys/kernel/debug/config_debug .
>
> 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.
>
> The compiler is smart enough to really trim down the code when
> the array is empty. An objdump -d looks like this:
>
> lib/perf-configs.o: file format elf64-x86-64
>
> Disassembly of section .init.text:
>
> 0000000000000000 <performance_taint>:
> 0: 55 push %rbp
> 1: 31 c0 xor %eax,%eax
> 3: 48 89 e5 mov %rsp,%rbp
> 6: 5d pop %rbp
> 7: c3 retq
So I guess the _taint bit is obsolete now?
> 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.
>
> For anybody that *really* cares, I put the whole thing under
> CONFIG_DEBUG_KERNEL in the Makefile.
>
> The messages look like this:
>
> [ 3.865297] WARNING: Do not use this kernel for performance measurement.
I'd warn this way:
[ 3.865297] INFO: Be careful when using this kernel for performance measurement.
> [ 3.868776] WARNING: Potentially performance-altering options:
> [ 3.871558] CONFIG_LOCKDEP enabled
> [ 3.873326] CONFIG_SLUB_DEBUG_ON enabled
And here I'd print this the following way:
> [ 3.868776] INFO: Potentially performance-altering options:
> [ 3.871558] CONFIG_LOCKDEP=y
> [ 3.873326] CONFIG_SLUB_DEBUG_ON=y
The 'INFO:' prefix is less pushy, and the '=y' notation is what
people will look for in the .config anyway, so lets keep to
that?
(Btw., you probably want to check CONFIG_PROVE_LOCKING=y, not
CONFIG_LOCKDEP - the former is the user configurable one.)
>
> 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 | 1
> b/lib/Makefile | 1
> b/lib/perf-configs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 117 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
That's unnecessary now.
>
> 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-20 09:56:29.528471033 -0700
> @@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
> { TAINT_OOT_MODULE, 'O', ' ' },
> { TAINT_UNSIGNED_MODULE, 'E', ' ' },
> { TAINT_SOFTLOCKUP, 'L', ' ' },
> + { TAINT_PERFORMANCE, 'Q', ' ' },
Ditto.
> };
>
> /**
> diff -puN /dev/null lib/perf-configs.c
> --- /dev/null 2014-04-10 11:28:14.066815724 -0700
> +++ b/lib/perf-configs.c 2014-08-21 13:22:25.586598278 -0700
> @@ -0,0 +1,114 @@
> +#include <linux/bug.h>
> +#include <linux/debugfs.h>
> +#include <linux/gfp.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +/*
> + * This should list any kernel options that can substantially
> + * affect performance. This is intended to give a loud
> + * warning during bootup so that folks have a fighting chance
> + * of noticing these things.
> + *
> + * This is fairly subjective, but a good rule of thumb for these
> + * is: if it is enabled widely in production, then it does not
> + * belong here. If a major enterprise kernel enables a feature
> + * for a non-debug kernel, it _really_ does not belong.
> + */
> +static const char * const perfomance_killing_configs[] = {
> +#ifdef CONFIG_LOCKDEP
> + "LOCKDEP",
> +#endif
> +#ifdef CONFIG_LOCK_STAT
> + "LOCK_STAT",
> +#endif
> +#ifdef CONFIG_DEBUG_VM
> + "DEBUG_VM",
> +#endif
> +#ifdef CONFIG_DEBUG_VM_VMACACHE
> + "DEBUG_VM_VMACACHE",
> +#endif
> +#ifdef CONFIG_DEBUG_VM_RB
> + "DEBUG_VM_RB",
> +#endif
> +#ifdef CONFIG_DEBUG_SLAB
> + "DEBUG_SLAB",
> +#endif
> +#ifdef CONFIG_SLUB_DEBUG_ON
> + "SLUB_DEBUG_ON",
> +#endif
> +#ifdef CONFIG_DEBUG_OBJECTS_FREE
> + "DEBUG_OBJECTS_FREE",
> +#endif
Essentially all DEBUG_OBJECTS_* options are expensive, assuming
they are enabled, i.e. DEBUG_OBJECTS_ENABLE_DEFAULT=y.
Otherwise they should only be warned about if the debugobjects
boot option got enabled.
I.e. you'll need a bit of a runtime check for this one.
> +#ifdef CONFIG_DEBUG_KMEMLEAK
> + "DEBUG_KMEMLEAK",
> +#endif
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> + "DEBUG_PAGEALLOC",
> +#endif
I'd also add KMEMCHECK.
> +};
> +
> +static const char config_prefix[] = "CONFIG_";
> +/*
> + * Dump out the list of the offending config options to a file
> + * in debugfs so that tooling can look for and capture it.
> + */
> +static ssize_t performance_taint_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + int i;
> + int ret;
> + char *buf;
> + size_t buf_written = 0;
> + size_t buf_left;
> + size_t buf_len;
> +
> + if (!ARRAY_SIZE(perfomance_killing_configs))
> + return 0;
> +
> + buf_len = 1;
> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++)
> + buf_len += strlen(config_prefix) +
> + strlen(perfomance_killing_configs[i]);
> + /* Add a byte for for each entry in the array for a \n */
> + buf_len += ARRAY_SIZE(perfomance_killing_configs);
> +
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + buf_left = buf_len;
> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
> + buf_written += snprintf(buf + buf_written, buf_left,
> + "%s%s\n", config_prefix,
> + perfomance_killing_configs[i]);
> + buf_left = buf_len - buf_written;
So, ARRAY_SIZE(performance_killing_configs) is written out four
times, a temporary variable would be in order I suspect.
Also, do you want to check buf_left and break out early from
the loop if it goes non-positive?
> + }
> + ret = simple_read_from_buffer(user_buf, buf_written, ppos, buf, buf_len);
> + kfree(buf);
> + return ret;
> +}
> +
> +static const struct file_operations fops_perf_taint = {
> + .read = performance_taint_read,
> + .llseek = default_llseek,
> +};
> +
> +static int __init performance_taint(void)
I'd not name this 'taint' anymore, but check_configs() or so.
> +{
> + int i;
> +
> + if (!ARRAY_SIZE(perfomance_killing_configs))
> + return 0;
and: s/perfomance/performance across the whole file.
> +
> + pr_warn("WARNING: Do not use this kernel for performance measurement.\n");
> + pr_warn("WARNING: Potentially performance-altering options:\n");
> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
> + pr_warn("\t%s%s enabled\n", config_prefix,
> + perfomance_killing_configs[i]);
> + }
> + debugfs_create_file("config_debug", S_IRUSR | S_IWUSR,
> + NULL, NULL, &fops_perf_taint);
> + return 0;
> +}
> +late_initcall(performance_taint);
> diff -puN lib/Makefile~taint-performance lib/Makefile
> --- a/lib/Makefile~taint-performance 2014-08-20 11:02:54.130548350 -0700
> +++ b/lib/Makefile 2014-08-20 11:06:18.231744868 -0700
> @@ -54,6 +54,7 @@ obj-$(CONFIG_GENERIC_HWEIGHT) += hweight
> obj-$(CONFIG_BTREE) += btree.o
> obj-$(CONFIG_INTERVAL_TREE) += interval_tree.o
> obj-$(CONFIG_ASSOCIATIVE_ARRAY) += assoc_array.o
> +obj-$(CONFIG_DEBUG_KERNEL) += perf-configs.o
Please don't name it perf-*.c, that confuses it with perf
events.
Maybe name it check-configs.c or so?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-22 7:20 ` Ingo Molnar
@ 2014-08-22 15:02 ` Dave Hansen
2014-08-24 14:49 ` Ingo Molnar
0 siblings, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2014-08-22 15:02 UTC (permalink / raw
To: Ingo Molnar
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm,
cl, penberg, linux-mm, kirill, lauraa, Linus Torvalds,
Peter Zijlstra, Thomas Gleixner
On 08/22/2014 12:20 AM, Ingo Molnar wrote:
> Essentially all DEBUG_OBJECTS_* options are expensive, assuming
> they are enabled, i.e. DEBUG_OBJECTS_ENABLE_DEFAULT=y.
>
> Otherwise they should only be warned about if the debugobjects
> boot option got enabled.
>
> I.e. you'll need a bit of a runtime check for this one.
At that point, what do we print, and when do we print it? We're not
saying that the config option should be disabled because it's really the
boot option plus the config option that is causing the problem.
I'll just put the DEBUG_OBJECTS_ENABLE_DEFAULT in here which is
analogous to what we're doing with SLUB_DEBUG_ON.
>> +static ssize_t performance_taint_read(struct file *file, char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int i;
>> + int ret;
>> + char *buf;
>> + size_t buf_written = 0;
>> + size_t buf_left;
>> + size_t buf_len;
>> +
>> + if (!ARRAY_SIZE(perfomance_killing_configs))
>> + return 0;
>> +
>> + buf_len = 1;
>> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++)
>> + buf_len += strlen(config_prefix) +
>> + strlen(perfomance_killing_configs[i]);
>> + /* Add a byte for for each entry in the array for a \n */
>> + buf_len += ARRAY_SIZE(perfomance_killing_configs);
>> +
>> + buf = kmalloc(buf_len, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + buf_left = buf_len;
>> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
>> + buf_written += snprintf(buf + buf_written, buf_left,
>> + "%s%s\n", config_prefix,
>> + perfomance_killing_configs[i]);
>> + buf_left = buf_len - buf_written;
>
> So, ARRAY_SIZE(performance_killing_configs) is written out four
> times, a temporary variable would be in order I suspect.
If one of them had gone over 80 chars, I probably would have. :) I put
one in anyway.
> Also, do you want to check buf_left and break out early from
> the loop if it goes non-positive?
You're slowly inflating my patch for no practical gain. :)
Oh well, I put that in too.
I think I got the rest of the things addressed too. v4 coming soon.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-21 20:24 [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE Dave Hansen
2014-08-21 20:57 ` Dave Jones
2014-08-22 7:20 ` Ingo Molnar
@ 2014-08-22 16:32 ` Tim Chen
2014-08-22 16:45 ` Dave Hansen
2 siblings, 1 reply; 10+ messages in thread
From: Tim Chen @ 2014-08-22 16:32 UTC (permalink / raw
To: Dave Hansen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, akpm, cl, penberg,
linux-mm, kirill, lauraa
[-- Attachment #1: Type: text/plain, Size: 5680 bytes --]
On Thu, 2014-08-21 at 13:24 -0700, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> Changes from v2:
> * remove tainting and stack track
> * add debugfs file
> * added a little text to guide folks who want to add more
> options
>
> 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. We can also teach tooling to
> look for and capture /sys/kernel/debug/config_debug .
>
> 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.
>
> The compiler is smart enough to really trim down the code when
> the array is empty. An objdump -d looks like this:
>
> lib/perf-configs.o: file format elf64-x86-64
>
> Disassembly of section .init.text:
>
> 0000000000000000 <performance_taint>:
> 0: 55 push %rbp
> 1: 31 c0 xor %eax,%eax
> 3: 48 89 e5 mov %rsp,%rbp
> 6: 5d pop %rbp
> 7: c3 retq
>
> 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.
>
> For anybody that *really* cares, I put the whole thing under
> CONFIG_DEBUG_KERNEL in the Makefile.
>
> The messages look like this:
>
> [ 3.865297] WARNING: Do not use this kernel for performance measurement.
> [ 3.868776] WARNING: Potentially performance-altering options:
> [ 3.871558] CONFIG_LOCKDEP enabled
> [ 3.873326] CONFIG_SLUB_DEBUG_ON enabled
>
> 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 | 1
> b/lib/Makefile | 1
> b/lib/perf-configs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 117 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-20 09:56:29.528471033 -0700
> @@ -225,6 +225,7 @@ static const struct tnt tnts[] = {
> { TAINT_OOT_MODULE, 'O', ' ' },
> { TAINT_UNSIGNED_MODULE, 'E', ' ' },
> { TAINT_SOFTLOCKUP, 'L', ' ' },
> + { TAINT_PERFORMANCE, 'Q', ' ' },
> };
>
> /**
> diff -puN /dev/null lib/perf-configs.c
> --- /dev/null 2014-04-10 11:28:14.066815724 -0700
> +++ b/lib/perf-configs.c 2014-08-21 13:22:25.586598278 -0700
> @@ -0,0 +1,114 @@
> +#include <linux/bug.h>
> +#include <linux/debugfs.h>
> +#include <linux/gfp.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +/*
> + * This should list any kernel options that can substantially
> + * affect performance. This is intended to give a loud
> + * warning during bootup so that folks have a fighting chance
> + * of noticing these things.
> + *
> + * This is fairly subjective, but a good rule of thumb for these
> + * is: if it is enabled widely in production, then it does not
> + * belong here. If a major enterprise kernel enables a feature
> + * for a non-debug kernel, it _really_ does not belong.
> + */
> +static const char * const perfomance_killing_configs[] = {
> +#ifdef CONFIG_LOCKDEP
> + "LOCKDEP",
> +#endif
> +#ifdef CONFIG_LOCK_STAT
> + "LOCK_STAT",
> +#endif
> +#ifdef CONFIG_DEBUG_VM
> + "DEBUG_VM",
> +#endif
> +#ifdef CONFIG_DEBUG_VM_VMACACHE
> + "DEBUG_VM_VMACACHE",
> +#endif
> +#ifdef CONFIG_DEBUG_VM_RB
> + "DEBUG_VM_RB",
> +#endif
> +#ifdef CONFIG_DEBUG_SLAB
> + "DEBUG_SLAB",
> +#endif
> +#ifdef CONFIG_SLUB_DEBUG_ON
> + "SLUB_DEBUG_ON",
> +#endif
> +#ifdef CONFIG_DEBUG_OBJECTS_FREE
> + "DEBUG_OBJECTS_FREE",
> +#endif
> +#ifdef CONFIG_DEBUG_KMEMLEAK
> + "DEBUG_KMEMLEAK",
> +#endif
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> + "DEBUG_PAGEALLOC",
I think coverage profiling also impact performance.
So I sould also put CONFIG_GCOV_KERNEL in the list.
Tim
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-22 16:32 ` Tim Chen
@ 2014-08-22 16:45 ` Dave Hansen
2014-08-22 18:12 ` Tim Chen
0 siblings, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2014-08-22 16:45 UTC (permalink / raw
To: Tim Chen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, akpm, cl, penberg,
linux-mm, kirill, lauraa
On 08/22/2014 09:32 AM, Tim Chen wrote:
>> > +#ifdef CONFIG_DEBUG_OBJECTS_FREE
>> > + "DEBUG_OBJECTS_FREE",
>> > +#endif
>> > +#ifdef CONFIG_DEBUG_KMEMLEAK
>> > + "DEBUG_KMEMLEAK",
>> > +#endif
>> > +#ifdef CONFIG_DEBUG_PAGEALLOC
>> > + "DEBUG_PAGEALLOC",
> I think coverage profiling also impact performance.
> So I sould also put CONFIG_GCOV_KERNEL in the list.
Would CONFIG_GCOV_PROFILE_ALL be the better one to check? With plain
GCOV_KERNEL, I don't think we will, by default, put the coverage
information in any files and slow them down.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-22 16:45 ` Dave Hansen
@ 2014-08-22 18:12 ` Tim Chen
0 siblings, 0 replies; 10+ messages in thread
From: Tim Chen @ 2014-08-22 18:12 UTC (permalink / raw
To: Dave Hansen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, akpm, cl, penberg,
linux-mm, kirill, lauraa
On Fri, 2014-08-22 at 09:45 -0700, Dave Hansen wrote:
> On 08/22/2014 09:32 AM, Tim Chen wrote:
> >> > +#ifdef CONFIG_DEBUG_OBJECTS_FREE
> >> > + "DEBUG_OBJECTS_FREE",
> >> > +#endif
> >> > +#ifdef CONFIG_DEBUG_KMEMLEAK
> >> > + "DEBUG_KMEMLEAK",
> >> > +#endif
> >> > +#ifdef CONFIG_DEBUG_PAGEALLOC
> >> > + "DEBUG_PAGEALLOC",
> > I think coverage profiling also impact performance.
> > So I sould also put CONFIG_GCOV_KERNEL in the list.
>
> Would CONFIG_GCOV_PROFILE_ALL be the better one to check? With plain
> GCOV_KERNEL, I don't think we will, by default, put the coverage
> information in any files and slow them down.
CONFIG_GCOV_PROFILE_ALL is definitely a no no regarding to
performance impact, which is mentioned in the gcov documentation.
I haven't tested this, but if profiling is turned on only for
a piece of code that is performance critical but not for
the whole kernel, in theory performance can still be impacted
with the overhead. So I think it is safer to check
for CONFIG_GCOV_KERNEL, that has no reason to be turned on
for any workload that's performance critical.
Tim
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-22 15:02 ` Dave Hansen
@ 2014-08-24 14:49 ` Ingo Molnar
2014-08-24 20:40 ` Dave Hansen
0 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2014-08-24 14:49 UTC (permalink / raw
To: Dave Hansen
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm,
cl, penberg, linux-mm, kirill, lauraa, Linus Torvalds,
Peter Zijlstra, Thomas Gleixner
* Dave Hansen <dave@sr71.net> wrote:
> On 08/22/2014 12:20 AM, Ingo Molnar wrote:
> > Essentially all DEBUG_OBJECTS_* options are expensive, assuming
> > they are enabled, i.e. DEBUG_OBJECTS_ENABLE_DEFAULT=y.
> >
> > Otherwise they should only be warned about if the debugobjects
> > boot option got enabled.
> >
> > I.e. you'll need a bit of a runtime check for this one.
>
> At that point, what do we print, and when do we print it? We're not
> saying that the config option should be disabled because it's really the
> boot option plus the config option that is causing the problem.
>
> I'll just put the DEBUG_OBJECTS_ENABLE_DEFAULT in here which is
> analogous to what we're doing with SLUB_DEBUG_ON.
>
> >> +static ssize_t performance_taint_read(struct file *file, char __user *user_buf,
> >> + size_t count, loff_t *ppos)
> >> +{
> >> + int i;
> >> + int ret;
> >> + char *buf;
> >> + size_t buf_written = 0;
> >> + size_t buf_left;
> >> + size_t buf_len;
> >> +
> >> + if (!ARRAY_SIZE(perfomance_killing_configs))
> >> + return 0;
> >> +
> >> + buf_len = 1;
> >> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++)
> >> + buf_len += strlen(config_prefix) +
> >> + strlen(perfomance_killing_configs[i]);
> >> + /* Add a byte for for each entry in the array for a \n */
> >> + buf_len += ARRAY_SIZE(perfomance_killing_configs);
> >> +
> >> + buf = kmalloc(buf_len, GFP_KERNEL);
> >> + if (!buf)
> >> + return -ENOMEM;
> >> +
> >> + buf_left = buf_len;
> >> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
> >> + buf_written += snprintf(buf + buf_written, buf_left,
> >> + "%s%s\n", config_prefix,
> >> + perfomance_killing_configs[i]);
> >> + buf_left = buf_len - buf_written;
> >
> > So, ARRAY_SIZE(performance_killing_configs) is written out four
> > times, a temporary variable would be in order I suspect.
>
> If one of them had gone over 80 chars, I probably would have. :) I put
> one in anyway.
>
> > Also, do you want to check buf_left and break out early from
> > the loop if it goes non-positive?
>
> You're slowly inflating my patch for no practical gain. :)
AFAICS it's a potential memory corruption and security bug,
should the array ever grow large enough to overflow the passed
in buffer size.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE
2014-08-24 14:49 ` Ingo Molnar
@ 2014-08-24 20:40 ` Dave Hansen
0 siblings, 0 replies; 10+ messages in thread
From: Dave Hansen @ 2014-08-24 20:40 UTC (permalink / raw
To: Ingo Molnar
Cc: linux-kernel, dave.hansen, peterz, mingo, ak, tim.c.chen, akpm,
cl, penberg, linux-mm, kirill, lauraa, Linus Torvalds,
Peter Zijlstra, Thomas Gleixner
On 08/24/2014 07:49 AM, Ingo Molnar wrote:
>>>> > >> + buf_left = buf_len;
>>>> > >> + for (i = 0; i < ARRAY_SIZE(perfomance_killing_configs); i++) {
>>>> > >> + buf_written += snprintf(buf + buf_written, buf_left,
>>>> > >> + "%s%s\n", config_prefix,
>>>> > >> + perfomance_killing_configs[i]);
>>>> > >> + buf_left = buf_len - buf_written;
...
>>> > > Also, do you want to check buf_left and break out early from
>>> > > the loop if it goes non-positive?
>> >
>> > You're slowly inflating my patch for no practical gain. :)
> AFAICS it's a potential memory corruption and security bug,
> should the array ever grow large enough to overflow the passed
> in buffer size.
Let's say there is 1 "buf_left" and I attempt a 100-byte snprintf().
Won't snprintf() return 1, and buf_written will then equal buf_len?
buf_left=0 at that point, and will get passed in to the next snprintf()
as the buffer length. I'm expecting snprintf() to just return 0 when it
gets a 0 for its 'size'.
Exhausting the buffer will, at worst, mean a bunch of useless calls to
snprintf() that do nothing, but I don't think it will run over the end
of the buffer.
Or am I missing something?
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-08-24 20:41 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-21 20:24 [PATCH] [v3] warn on performance-impacting configs aka. TAINT_PERFORMANCE Dave Hansen
2014-08-21 20:57 ` Dave Jones
2014-08-21 21:03 ` Dave Hansen
2014-08-22 7:20 ` Ingo Molnar
2014-08-22 15:02 ` Dave Hansen
2014-08-24 14:49 ` Ingo Molnar
2014-08-24 20:40 ` Dave Hansen
2014-08-22 16:32 ` Tim Chen
2014-08-22 16:45 ` Dave Hansen
2014-08-22 18:12 ` Tim Chen
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 read-only IMAP folder(s) and NNTP newsgroup(s).