* Dave Hansen [off-list ref] 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.)

That's unnecessary now.

Ditto.

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.

Please don't name it perf-*.c, that confuses it with perf 
events.

Maybe name it check-configs.c or so?

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>