Add simple way to add statistic counters to debugfs

Add simple way to add statistic counters to debugfs

[Andi Kleen]

This extends debugfs to make it very simple to add per cpu counters
statistics that are exported there. This is useful for all kind
of debugging situations. The counters have very little overhead
and are simpler to use than trace points.

This is the infrastructure for debugfs and a demo application 
to instrument the dcache RCU which I developed some time ago
to track down some problems there.

Posting here also so that Fengguang can use it.

Module support is still missing, but could be added without too 
much work.

-Andi

[PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Allow passing path names to debugfs_create* and automatically create
the parents. This makes it much simpler for the caller to create hierarchies.

The way the reference counts are handled is admittedly a bit ugly.

There is no way to clean them up currently other than to delete the tree,
but that doesn't seem like a big problem for debugfs to leave
behind a few empty directories.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/inode.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f3a257d..7ea9a6c 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -146,6 +146,52 @@ static struct file_system_type debug_fs_type = {
 	.kill_sb =	kill_litter_super,
 };
 
+/*
+ * Created parents stay around later. Sorry, but it shouldn't be a big issue.
+ */
+static const char *create_parents(const char *name, struct dentry *parent,
+				  struct dentry **pp, int *ref)
+{
+	char fn[strlen(name) + 1];
+	char *tail;
+	char *s, *p;
+	struct dentry *new_parent, *dentry, *old_parent = NULL;
+
+	strcpy(fn, name);
+	tail = strrchr(fn, '/');
+	if (!tail)
+		return name;
+	tail++;
+
+	s = fn;
+	while ((p = strsep(&s, "/")) != NULL && p != tail) {
+		/* ignore error here */
+		dentry = debugfs_create_dir(p, parent);
+
+		/* Is it there now? */
+		mutex_lock(&parent->d_inode->i_mutex);
+		new_parent = lookup_one_len(p, parent, strlen(p));
+		mutex_unlock(&parent->d_inode->i_mutex);
+
+		if (!PTR_ERR(dentry))
+			dput(dentry);
+		if (IS_ERR(new_parent))
+			return NULL;
+		if (old_parent)
+			dput(old_parent);
+		if (parent != *pp)
+			old_parent = parent;
+		parent = new_parent;
+	}
+	if (parent) {
+		*ref = 1;
+		*pp = parent;
+	}
+	if (old_parent)
+		dput(old_parent);
+	return strrchr(name, '/') + 1;
+}
+
 static int debugfs_create_by_name(const char *name, mode_t mode,
 				  struct dentry *parent,
 				  struct dentry **dentry,
@@ -153,6 +199,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 				  const struct file_operations *fops)
 {
 	int error = 0;
+	int ref = 0;
 
 	/* If the parent is not specified, we create it in the root.
 	 * We need the root dentry to do this, which is in the super 
@@ -161,6 +208,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 	 */
 	if (!parent)
 		parent = debugfs_mount->mnt_sb->s_root;
+	name = create_parents(name, parent, &parent, &ref);
 
 	*dentry = NULL;
 	mutex_lock(&parent->d_inode->i_mutex);
@@ -185,6 +233,9 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 		error = PTR_ERR(*dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
 
+	if (ref)
+		dput(parent);
+
 	return error;
 }
 
-- 
1.7.4.4

Re: [PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files

[Greg KH]

On Fri, Dec 02, 2011 at 10:43:25AM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Allow passing path names to debugfs_create* and automatically create
> the parents. This makes it much simpler for the caller to create hierarchies.
> 
> The way the reference counts are handled is admittedly a bit ugly.
> 
> There is no way to clean them up currently other than to delete the tree,
> but that doesn't seem like a big problem for debugfs to leave
> behind a few empty directories.

I like this, but some documentation will probably need to be added
somwhere saying that it's now legal to create a debugfs file with
"this/is/a/tree" and have it all be expanded out.

And yes, cleaning up the directories shouldn't be a big issue, but what
if the directory is already there and it tries to be created again?
Shouldn't you do a lookup first and use that dentry if it's there?

thanks,

greg k-h

Re: [PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files

[Andi Kleen]

On Fri, Dec 02, 2011 at 11:17:24AM -0800, Greg KH wrote:
> I like this, but some documentation will probably need to be added
> somwhere saying that it's now legal to create a debugfs file with
> "this/is/a/tree" and have it all be expanded out.

Ok. Updating the kerneldoc entries.

> 
> And yes, cleaning up the directories shouldn't be a big issue, but what
> if the directory is already there and it tries to be created again?
> Shouldn't you do a lookup first and use that dentry if it's there?

In this case debugfs_create_dir() errors out, the caller ignores
the error and just looks it up. So yes should work.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files

[Greg KH]

On Fri, Dec 02, 2011 at 08:42:19PM +0100, Andi Kleen wrote:
> On Fri, Dec 02, 2011 at 11:17:24AM -0800, Greg KH wrote:
> > I like this, but some documentation will probably need to be added
> > somwhere saying that it's now legal to create a debugfs file with
> > "this/is/a/tree" and have it all be expanded out.
> 
> Ok. Updating the kerneldoc entries.
> 
> > 
> > And yes, cleaning up the directories shouldn't be a big issue, but what
> > if the directory is already there and it tries to be created again?
> > Shouldn't you do a lookup first and use that dentry if it's there?
> 
> In this case debugfs_create_dir() errors out, the caller ignores
> the error and just looks it up. So yes should work.

Ah, ok, nevermind then :)

thanks,

greg k-h

[PATCH 2/3] DEBUGFS: Add per cpu counters

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a simple way to export per cpu counters in debugfs.

This is done using a section, so that they can be declared using
a simple macro with minimal typing.

OPEN: modules are not supported yet.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/Makefile               |    2 +-
 fs/debugfs/counter.c              |   73 +++++++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |    6 +++
 include/linux/debugfs.h           |   27 ++++++++++++++
 4 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 fs/debugfs/counter.c

diff --git a/fs/debugfs/Makefile b/fs/debugfs/Makefile
index 840c456..21be8d3 100644
--- a/fs/debugfs/Makefile
+++ b/fs/debugfs/Makefile
@@ -1,4 +1,4 @@
-debugfs-objs	:= inode.o file.o
+debugfs-objs	:= inode.o file.o counter.o
 
 obj-$(CONFIG_DEBUG_FS)	+= debugfs.o
 
diff --git a/fs/debugfs/counter.c b/fs/debugfs/counter.c
new file mode 100644
index 0000000..2372faf
--- /dev/null
+++ b/fs/debugfs/counter.c
@@ -0,0 +1,73 @@
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/percpu.h>
+#include <linux/kernel.h>
+
+/* OPEN: implement module support */
+
+extern struct debugfs_counter __start___debugfs[], __stop___debugfs[];
+
+static unsigned sum_counter(unsigned __percpu *counter)
+{
+	int cpu;
+	unsigned sum = 0;
+
+	for_each_online_cpu (cpu)
+		sum += per_cpu(*counter, cpu);
+	return sum;
+}
+
+static int
+dump_counters(struct seq_file *m,
+	      struct debugfs_counter *cnt, struct debugfs_counter *stop,
+	      char *fn)
+{
+	int n = 0;
+	for (; cnt < stop; cnt++)
+		if (fn[0] == cnt->fn[0] && !strcmp(cnt->fn, fn))
+			n += seq_printf(m, "%s: %u\n",
+					cnt->name, sum_counter(cnt->ptr));
+	return n;
+}
+
+static int show_debugfs_counter(struct seq_file *m, void *arg)
+{
+	int n;
+	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);
+	return n;
+}
+
+static int debugfs_counter_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, show_debugfs_counter, inode->i_private);
+}
+
+static const struct file_operations debugfs_counter_fops = {
+	.open = debugfs_counter_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static void
+init_counters(struct debugfs_counter *cnt, struct debugfs_counter *stop)
+{
+	const char *last = NULL;
+
+	for (; cnt < stop; cnt++) {
+		if (cnt->fn == last)
+			continue;
+		last = cnt->fn;
+
+		/* Ignore error. Nothing we can do. */
+		debugfs_create_file(cnt->fn, 0444, NULL, (void *)cnt->fn,
+				    &debugfs_counter_fops);
+	}
+}
+
+static int init_debugfs_counters(void)
+{
+	init_counters(__start___debugfs, __stop___debugfs);
+	return 0;
+}
+module_init(init_debugfs_counters);
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..fbfb95b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -272,6 +272,12 @@
 									\
 	TRACEDATA							\
 									\
+	__debugfs  	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
+		VMLINUX_SYMBOL(__start___debugfs) = .;			\
+		*(__debugfs)						\
+		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
+	}								\
+									\
 	/* Kernel symbol table: Normal symbols */			\
 	__ksymtab         : AT(ADDR(__ksymtab) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start___ksymtab) = .;			\
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index e7d9b20..a243b05 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 
 #include <linux/types.h>
+#include <linux/percpu.h>
 
 struct file_operations;
 
@@ -76,6 +77,24 @@ struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 
 bool debugfs_initialized(void);
 
+struct debugfs_counter {
+	unsigned __percpu *ptr;
+	const char *fn;
+	const char *name;
+} __attribute__((aligned(sizeof(char *))));
+
+/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
+#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
+	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
+	struct debugfs_counter name_ ## _pcpu_counter __used		\
+	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
+	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \
+
+#define DECLARE_DEBUGFS_COUNTER(name)			\
+	struct debugfs_counter name ## _pcpu_counter
+
+#define debugfs_counter_inc(name) this_cpu_inc(name ## _counter)
+
 #else
 
 #include <linux/err.h>
@@ -193,6 +212,14 @@ static inline bool debugfs_initialized(void)
 	return false;
 }
 
+
+struct debugfs_counter {
+};
+
+#define DEFINE_DEBUGFS_COUNTER(name, file)
+#define DECLARE_DEBUGFS_COUNTER(name)
+#define debugfs_counter_inc(name) do {} while(0)
+
 #endif
 
 #endif
-- 
1.7.4.4

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Wu Fengguang]

On Sat, Dec 03, 2011 at 02:43:26AM +0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add a simple way to export per cpu counters in debugfs.
> 
> This is done using a section, so that they can be declared using
> a simple macro with minimal typing.
> 
> OPEN: modules are not supported yet.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  fs/debugfs/Makefile               |    2 +-
>  fs/debugfs/counter.c              |   73 +++++++++++++++++++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h |    6 +++
>  include/linux/debugfs.h           |   27 ++++++++++++++
>  4 files changed, 107 insertions(+), 1 deletions(-)
>  create mode 100644 fs/debugfs/counter.c
> 
> diff --git a/fs/debugfs/Makefile b/fs/debugfs/Makefile
> index 840c456..21be8d3 100644
> --- a/fs/debugfs/Makefile
> +++ b/fs/debugfs/Makefile
> @@ -1,4 +1,4 @@
> -debugfs-objs	:= inode.o file.o
> +debugfs-objs	:= inode.o file.o counter.o
>  
>  obj-$(CONFIG_DEBUG_FS)	+= debugfs.o
>  
> diff --git a/fs/debugfs/counter.c b/fs/debugfs/counter.c
> new file mode 100644
> index 0000000..2372faf
> --- /dev/null
> +++ b/fs/debugfs/counter.c
> @@ -0,0 +1,73 @@
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/percpu.h>
> +#include <linux/kernel.h>
> +
> +/* OPEN: implement module support */

Yeah, I think the module support would benefit my case as well.

To support module users, init_counters() will be exported with the
__start___debugfs/__stop___debugfs hard coding removed. Then I'll be
call it from the readahead initilization code:

DEFINE_PER_CPU(unsigned long[RA_PATTERN_MAX][RA_ACCOUNT_MAX], ra_counter);
struct debugfs_counter ra_pcpu_counter[RA_PATTERN_MAX][RA_ACCOUNT_MAX];

// init ra_pcpu_counter in a loop
for each pattern
        init_counters(ra_pcpu_counter[pattern], ra_pcpu_counter[pattern+1]);

Does this usage sound reasonable?

> +extern struct debugfs_counter __start___debugfs[], __stop___debugfs[];
> +
> +static unsigned sum_counter(unsigned __percpu *counter)
> +{
> +	int cpu;
> +	unsigned sum = 0;
> +
> +	for_each_online_cpu (cpu)
> +		sum += per_cpu(*counter, cpu);
> +	return sum;
> +}
> +
> +static int
> +dump_counters(struct seq_file *m,
> +	      struct debugfs_counter *cnt, struct debugfs_counter *stop,
> +	      char *fn)
> +{
> +	int n = 0;
> +	for (; cnt < stop; cnt++)
> +		if (fn[0] == cnt->fn[0] && !strcmp(cnt->fn, fn))
> +			n += seq_printf(m, "%s: %u\n",
> +					cnt->name, sum_counter(cnt->ptr));
> +	return n;
> +}
> +
> +static int show_debugfs_counter(struct seq_file *m, void *arg)
> +{
> +	int n;
> +	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);

That hard coded __start___debugfs/__stop___debugfs is OK for POC, and
will need to be improved to work with multiple users in kernel.

Thanks,
Fengguang

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Andi Kleen]

On Tue, Dec 06, 2011 at 07:44:38PM +0800, Wu Fengguang wrote:
> > +#include <linux/seq_file.h>
> > +#include <linux/percpu.h>
> > +#include <linux/kernel.h>
> > +
> > +/* OPEN: implement module support */
> 
> Yeah, I think the module support would benefit my case as well.
> 
> To support module users, init_counters() will be exported with the
> __start___debugfs/__stop___debugfs hard coding removed. Then I'll be
> call it from the readahead initilization code:

No, the module loader should take care of this: look for the magic
section, register it. This is already done for other magic
sections and not too difficult, i just didn't write the code
so far.

Then the DEFINE_* macros would work seamlessly in modules too.

Do you really need it for the readahead code? I thought that
was builtin.


> 
> DEFINE_PER_CPU(unsigned long[RA_PATTERN_MAX][RA_ACCOUNT_MAX], ra_counter);
> struct debugfs_counter ra_pcpu_counter[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
> 
> // init ra_pcpu_counter in a loop
> for each pattern
>         init_counters(ra_pcpu_counter[pattern], ra_pcpu_counter[pattern+1]);

Ok so you need arrays.

The idea is to not call some init function, but just put it into section
and let the init code walk it.

Should probably have a macro that handles arrays nicely.

> > +static int show_debugfs_counter(struct seq_file *m, void *arg)
> > +{
> > +	int n;
> > +	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);
> 
> That hard coded __start___debugfs/__stop___debugfs is OK for POC, and
> will need to be improved to work with multiple users in kernel.

For Modules it obviously has to walk a list. It's a straight forward extension.

I don't think it should allow everyone to register their own lists,
that doesn't make sense because they could as well create the debug 
files themselves.

-Andi

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Wu Fengguang]

On Wed, Dec 07, 2011 at 01:17:51AM +0800, Andi Kleen wrote:
> On Tue, Dec 06, 2011 at 07:44:38PM +0800, Wu Fengguang wrote:
> > > +#include <linux/seq_file.h>
> > > +#include <linux/percpu.h>
> > > +#include <linux/kernel.h>
> > > +
> > > +/* OPEN: implement module support */
> > 
> > Yeah, I think the module support would benefit my case as well.
> > 
> > To support module users, init_counters() will be exported with the
> > __start___debugfs/__stop___debugfs hard coding removed. Then I'll be
> > call it from the readahead initilization code:
> 
> No, the module loader should take care of this: look for the magic
> section, register it. This is already done for other magic
> sections and not too difficult, i just didn't write the code
> so far.
>
> Then the DEFINE_* macros would work seamlessly in modules too.
 
OK.

> Do you really need it for the readahead code? I thought that
> was builtin.

Yeah because in the current form I'm initializing an array of counters
at runtime in the readahead call site. 

> > DEFINE_PER_CPU(unsigned long[RA_PATTERN_MAX][RA_ACCOUNT_MAX], ra_counter);
> > struct debugfs_counter ra_pcpu_counter[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
> > 
> > // init ra_pcpu_counter in a loop
> > for each pattern
> >         init_counters(ra_pcpu_counter[pattern], ra_pcpu_counter[pattern+1]);
> 
> Ok so you need arrays.
> 
> The idea is to not call some init function, but just put it into section
> and let the init code walk it.
> 
> Should probably have a macro that handles arrays nicely.

That would be nice!

> > > +static int show_debugfs_counter(struct seq_file *m, void *arg)
> > > +{
> > > +	int n;
> > > +	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);
> > 
> > That hard coded __start___debugfs/__stop___debugfs is OK for POC, and
> > will need to be improved to work with multiple users in kernel.
> 
> For Modules it obviously has to walk a list. It's a straight forward extension.
> 
> I don't think it should allow everyone to register their own lists,
> that doesn't make sense because they could as well create the debug 
> files themselves.

OK. It just looks a bit inefficient when (this becomes so popular that)
the list grows large.

Thanks,
Fengguang

[PATCH 3/3] VFS: Add event counting to dcache

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Most self respecting subsystems -- like networking or MM -- have
own counter infrastructure these days. This is useful to understand
the behaviour of a running system. Counters are low enough
overhead that they can be always enabled.

This patch adds event counts to the dcache.

Instead of developing an own counter infrastructure for the VFS
I'm using generic counters implemented in debugfs.

Since we had problems with this recently I instrumented the dcache
RCU code. This is rather tricky code which is difficult to tune,
and some indication on why aborts happen is quite useful.

I'm especially interested in feedback on the placement of the event counters.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/namei.c |   42 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 5008f01..bfbe36a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -33,10 +33,22 @@
 #include <linux/device_cgroup.h>
 #include <linux/fs_struct.h>
 #include <linux/posix_acl.h>
+#include <linux/debugfs.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
 
+const char dname[] = "vfs/dcache";
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_root_changed_abort, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_dir_changed_abort, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_entry_changed_abort, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_permission_abort, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_revalidate_abort, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_ref_walks, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_rcu_walks, dname);
+DEFINE_DEBUGFS_COUNTER(dcache_reval_walks, dname);
+
+
 /* [Feb-1997 T. Schoebel-Theuer]
  * Fundamental changes in the pathname lookup mechanisms (namei)
  * were necessary because of omirr.  The reason is that omirr needs
@@ -430,20 +442,26 @@ static int unlazy_walk(struct nameidata *nd, struct dentry *dentry)
 		want_root = 1;
 		spin_lock(&fs->lock);
 		if (nd->root.mnt != fs->root.mnt ||
-				nd->root.dentry != fs->root.dentry)
+		    nd->root.dentry != fs->root.dentry) {
+			debugfs_counter_inc(dcache_rcu_root_changed_abort);
 			goto err_root;
+		}
 	}
 	spin_lock(&parent->d_lock);
 	if (!dentry) {
-		if (!__d_rcu_to_refcount(parent, nd->seq))
+		if (!__d_rcu_to_refcount(parent, nd->seq)) {
+			debugfs_counter_inc(dcache_rcu_dir_changed_abort);
 			goto err_parent;
+		}
 		BUG_ON(nd->inode != parent->d_inode);
 	} else {
 		if (dentry->d_parent != parent)
 			goto err_parent;
 		spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
-		if (!__d_rcu_to_refcount(dentry, nd->seq))
+		if (!__d_rcu_to_refcount(dentry, nd->seq)) {
+			debugfs_counter_inc(dcache_rcu_entry_changed_abort);
 			goto err_child;
+		}
 		/*
 		 * If the sequence check on the child dentry passed, then
 		 * the child has not been removed from its parent. This
@@ -474,6 +492,7 @@ err_parent:
 err_root:
 	if (want_root)
 		spin_unlock(&fs->lock);
+	debugfs_counter_inc(dcache_rcu_root_changed_abort);
 	return -ECHILD;
 }
 
@@ -522,6 +541,7 @@ static int complete_walk(struct nameidata *nd)
 			spin_unlock(&dentry->d_lock);
 			rcu_read_unlock();
 			br_read_unlock(vfsmount_lock);
+			debugfs_counter_inc(dcache_rcu_entry_changed_abort);
 			return -ECHILD;
 		}
 		BUG_ON(nd->inode != dentry->d_inode);
@@ -960,6 +980,7 @@ failed:
 		nd->root.mnt = NULL;
 	rcu_read_unlock();
 	br_read_unlock(vfsmount_lock);
+	debugfs_counter_inc(dcache_rcu_entry_changed_abort);
 	return -ECHILD;
 }
 
@@ -1132,8 +1153,10 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 			goto unlazy;
 
 		/* Memory barrier in read_seqcount_begin of child is enough */
-		if (__read_seqcount_retry(&parent->d_seq, nd->seq))
+		if (__read_seqcount_retry(&parent->d_seq, nd->seq)) {
+			debugfs_counter_inc(dcache_rcu_dir_changed_abort);
 			return -ECHILD;
+		}
 		nd->seq = seq;
 
 		if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) {
@@ -1141,6 +1164,7 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 			if (unlikely(status <= 0)) {
 				if (status != -ECHILD)
 					need_reval = 0;
+				debugfs_counter_inc(dcache_rcu_revalidate_abort);
 				goto unlazy;
 			}
 		}
@@ -1226,6 +1250,7 @@ static inline int may_lookup(struct nameidata *nd)
 		int err = inode_permission(nd->inode, MAY_EXEC|MAY_NOT_BLOCK);
 		if (err != -ECHILD)
 			return err;
+		debugfs_counter_inc(dcache_rcu_permission_abort);
 		if (unlazy_walk(nd, NULL))
 			return -ECHILD;
 	}
@@ -1643,10 +1668,15 @@ static int do_path_lookup(int dfd, const char *name,
 				unsigned int flags, struct nameidata *nd)
 {
 	int retval = path_lookupat(dfd, name, flags | LOOKUP_RCU, nd);
-	if (unlikely(retval == -ECHILD))
+	debugfs_counter_inc(dcache_rcu_walks);
+	if (unlikely(retval == -ECHILD)) {
+		debugfs_counter_inc(dcache_ref_walks);
 		retval = path_lookupat(dfd, name, flags, nd);
-	if (unlikely(retval == -ESTALE))
+	}
+	if (unlikely(retval == -ESTALE)) {
+		debugfs_counter_inc(dcache_reval_walks);
 		retval = path_lookupat(dfd, name, flags | LOOKUP_REVAL, nd);
+	}
 
 	if (likely(!retval)) {
 		if (unlikely(!audit_dummy_context())) {
-- 
1.7.4.4

Re: Add simple way to add statistic counters to debugfs

[Greg KH]

On Fri, Dec 02, 2011 at 10:43:24AM -0800, Andi Kleen wrote:
> This extends debugfs to make it very simple to add per cpu counters
> statistics that are exported there. This is useful for all kind
> of debugging situations. The counters have very little overhead
> and are simpler to use than trace points.
> 
> This is the infrastructure for debugfs and a demo application 
> to instrument the dcache RCU which I developed some time ago
> to track down some problems there.
> 
> Posting here also so that Fengguang can use it.
> 
> Module support is still missing, but could be added without too 
> much work.

I like this a lot, very nice job.  I just had one comment with the first
patch.

And yes, I would like module support as well, but others can add that if
they really need it :)

Care to fix up the first patch and I'll be glad to take the two debugfs
patches and any others if you want me to.

thanks,

greg k-h

[PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Allow passing path names to debugfs_create* and automatically create
the parents. This makes it much simpler for the caller to create hierarchies.

The way the reference counts are handled is admittedly a bit ugly.

There is no way to clean them up currently other than to delete the tree,
but that doesn't seem like a big problem for debugfs to leave
behind a few empty directories.

v2: Comment fixes based on review comments.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/inode.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f3a257d..b66ab79 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -146,6 +146,55 @@ static struct file_system_type debug_fs_type = {
 	.kill_sb =	kill_litter_super,
 };
 
+/*
+ * Created parents stay around later. Sorry, but it shouldn't be a big issue.
+ */
+static const char *create_parents(const char *name, struct dentry *parent,
+				  struct dentry **pp, int *ref)
+{
+	char fn[strlen(name) + 1];
+	char *tail;
+	char *s, *p;
+	struct dentry *new_parent, *dentry, *old_parent = NULL;
+
+	strcpy(fn, name);
+	tail = strrchr(fn, '/');
+	if (!tail)
+		return name;
+	tail++;
+
+	s = fn;
+	while ((p = strsep(&s, "/")) != NULL && p != tail) {
+		/* 
+ 		 * When the parent already exists, this will error out.
+ 		 * Ignore that error. We'll just look the dentry up below.
+ 		 */
+		dentry = debugfs_create_dir(p, parent);
+
+		/* Is it there now? */
+		mutex_lock(&parent->d_inode->i_mutex);
+		new_parent = lookup_one_len(p, parent, strlen(p));
+		mutex_unlock(&parent->d_inode->i_mutex);
+
+		if (!PTR_ERR(dentry))
+			dput(dentry);
+		if (IS_ERR(new_parent))
+			return NULL;
+		if (old_parent)
+			dput(old_parent);
+		if (parent != *pp)
+			old_parent = parent;
+		parent = new_parent;
+	}
+	if (parent) {
+		*ref = 1;
+		*pp = parent;
+	}
+	if (old_parent)
+		dput(old_parent);
+	return strrchr(name, '/') + 1;
+}
+
 static int debugfs_create_by_name(const char *name, mode_t mode,
 				  struct dentry *parent,
 				  struct dentry **dentry,
@@ -153,6 +202,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 				  const struct file_operations *fops)
 {
 	int error = 0;
+	int ref = 0;
 
 	/* If the parent is not specified, we create it in the root.
 	 * We need the root dentry to do this, which is in the super 
@@ -161,6 +211,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 	 */
 	if (!parent)
 		parent = debugfs_mount->mnt_sb->s_root;
+	name = create_parents(name, parent, &parent, &ref);
 
 	*dentry = NULL;
 	mutex_lock(&parent->d_inode->i_mutex);
@@ -185,12 +236,17 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 		error = PTR_ERR(*dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
 
+	if (ref)
+		dput(parent);
+
 	return error;
 }
 
 /**
  * debugfs_create_file - create a file in the debugfs filesystem
  * @name: a pointer to a string containing the name of the file to create.
+ *        This can also contain directories (dir/file), in this case missing
+ *        directories will be automatically created.
  * @mode: the permission that the file should have.
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this paramater is NULL, then the
@@ -243,7 +299,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
 /**
  * debugfs_create_dir - create a directory in the debugfs filesystem
  * @name: a pointer to a string containing the name of the directory to
- *        create.
+ *        create. This can also contain directories (dir/file), in this case 
+ *        missing directories will be automatically created.
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this paramater is NULL, then the
  *          directory will be created in the root of the debugfs filesystem.
@@ -269,7 +326,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
 /**
  * debugfs_create_symlink- create a symbolic link in the debugfs filesystem
  * @name: a pointer to a string containing the name of the symbolic link to
- *        create.
+ *        create.  This can also contain directories (dir/file), in this case 
+ *        missing directories will be automatically created
  * @parent: a pointer to the parent dentry for this symbolic link.  This
  *          should be a directory dentry if set.  If this paramater is NULL,
  *          then the symbolic link will be created in the root of the debugfs
-- 
1.7.4.4

[PATCH 2/3] DEBUGFS: Add per cpu counters

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a simple way to export per cpu counters in debugfs.

This is done using a section, so that they can be declared using
a simple macro with minimal typing.

OPEN: modules are not supported yet.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/Makefile               |    2 +-
 fs/debugfs/counter.c              |   73 +++++++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |    6 +++
 include/linux/debugfs.h           |   27 ++++++++++++++
 4 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 fs/debugfs/counter.c

diff --git a/fs/debugfs/Makefile b/fs/debugfs/Makefile
index 840c456..21be8d3 100644
--- a/fs/debugfs/Makefile
+++ b/fs/debugfs/Makefile
@@ -1,4 +1,4 @@
-debugfs-objs	:= inode.o file.o
+debugfs-objs	:= inode.o file.o counter.o
 
 obj-$(CONFIG_DEBUG_FS)	+= debugfs.o
 
diff --git a/fs/debugfs/counter.c b/fs/debugfs/counter.c
new file mode 100644
index 0000000..2372faf
--- /dev/null
+++ b/fs/debugfs/counter.c
@@ -0,0 +1,73 @@
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/percpu.h>
+#include <linux/kernel.h>
+
+/* OPEN: implement module support */
+
+extern struct debugfs_counter __start___debugfs[], __stop___debugfs[];
+
+static unsigned sum_counter(unsigned __percpu *counter)
+{
+	int cpu;
+	unsigned sum = 0;
+
+	for_each_online_cpu (cpu)
+		sum += per_cpu(*counter, cpu);
+	return sum;
+}
+
+static int
+dump_counters(struct seq_file *m,
+	      struct debugfs_counter *cnt, struct debugfs_counter *stop,
+	      char *fn)
+{
+	int n = 0;
+	for (; cnt < stop; cnt++)
+		if (fn[0] == cnt->fn[0] && !strcmp(cnt->fn, fn))
+			n += seq_printf(m, "%s: %u\n",
+					cnt->name, sum_counter(cnt->ptr));
+	return n;
+}
+
+static int show_debugfs_counter(struct seq_file *m, void *arg)
+{
+	int n;
+	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);
+	return n;
+}
+
+static int debugfs_counter_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, show_debugfs_counter, inode->i_private);
+}
+
+static const struct file_operations debugfs_counter_fops = {
+	.open = debugfs_counter_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static void
+init_counters(struct debugfs_counter *cnt, struct debugfs_counter *stop)
+{
+	const char *last = NULL;
+
+	for (; cnt < stop; cnt++) {
+		if (cnt->fn == last)
+			continue;
+		last = cnt->fn;
+
+		/* Ignore error. Nothing we can do. */
+		debugfs_create_file(cnt->fn, 0444, NULL, (void *)cnt->fn,
+				    &debugfs_counter_fops);
+	}
+}
+
+static int init_debugfs_counters(void)
+{
+	init_counters(__start___debugfs, __stop___debugfs);
+	return 0;
+}
+module_init(init_debugfs_counters);
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..fbfb95b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -272,6 +272,12 @@
 									\
 	TRACEDATA							\
 									\
+	__debugfs  	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
+		VMLINUX_SYMBOL(__start___debugfs) = .;			\
+		*(__debugfs)						\
+		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
+	}								\
+									\
 	/* Kernel symbol table: Normal symbols */			\
 	__ksymtab         : AT(ADDR(__ksymtab) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start___ksymtab) = .;			\
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index e7d9b20..a243b05 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 
 #include <linux/types.h>
+#include <linux/percpu.h>
 
 struct file_operations;
 
@@ -76,6 +77,24 @@ struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 
 bool debugfs_initialized(void);
 
+struct debugfs_counter {
+	unsigned __percpu *ptr;
+	const char *fn;
+	const char *name;
+} __attribute__((aligned(sizeof(char *))));
+
+/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
+#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
+	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
+	struct debugfs_counter name_ ## _pcpu_counter __used		\
+	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
+	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \
+
+#define DECLARE_DEBUGFS_COUNTER(name)			\
+	struct debugfs_counter name ## _pcpu_counter
+
+#define debugfs_counter_inc(name) this_cpu_inc(name ## _counter)
+
 #else
 
 #include <linux/err.h>
@@ -193,6 +212,14 @@ static inline bool debugfs_initialized(void)
 	return false;
 }
 
+
+struct debugfs_counter {
+};
+
+#define DEFINE_DEBUGFS_COUNTER(name, file)
+#define DECLARE_DEBUGFS_COUNTER(name)
+#define debugfs_counter_inc(name) do {} while(0)
+
 #endif
 
 #endif
-- 
1.7.4.4

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Greg KH]

On Fri, Dec 02, 2011 at 12:02:47PM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add a simple way to export per cpu counters in debugfs.
> 
> This is done using a section, so that they can be declared using
> a simple macro with minimal typing.
> 
> OPEN: modules are not supported yet.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

After applying the previous patch, this one still needs "fuzz" which
makes git not happy, care to redo this one as well when you redo the 1/3
patch?

And I'm guessing that I don't need 3/3?

thanks,

greg k-h

[PATCH 1/3] DEBUGFS: Automatically create parents for debugfs files v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Allow passing path names to debugfs_create* and automatically create
the parents. This makes it much simpler for the caller to create hierarchies.

The way the reference counts are handled is admittedly a bit ugly.

There is no way to clean them up currently other than to delete the tree,
but that doesn't seem like a big problem for debugfs to leave
behind a few empty directories.

v2: Comment fixes based on review comments.
v3: satisfy the kernel community's white space obsession.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/inode.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f3a257d..0081ac0 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -146,6 +146,55 @@ static struct file_system_type debug_fs_type = {
 	.kill_sb =	kill_litter_super,
 };
 
+/*
+ * Created parents stay around later. Sorry, but it shouldn't be a big issue.
+ */
+static const char *create_parents(const char *name, struct dentry *parent,
+				  struct dentry **pp, int *ref)
+{
+	char fn[strlen(name) + 1];
+	char *tail;
+	char *s, *p;
+	struct dentry *new_parent, *dentry, *old_parent = NULL;
+
+	strcpy(fn, name);
+	tail = strrchr(fn, '/');
+	if (!tail)
+		return name;
+	tail++;
+
+	s = fn;
+	while ((p = strsep(&s, "/")) != NULL && p != tail) {
+		/*
+		 * When the parent already exists, this will error out.
+		 * Ignore that error. We'll just look the dentry up below.
+		 */
+		dentry = debugfs_create_dir(p, parent);
+
+		/* Is it there now? */
+		mutex_lock(&parent->d_inode->i_mutex);
+		new_parent = lookup_one_len(p, parent, strlen(p));
+		mutex_unlock(&parent->d_inode->i_mutex);
+
+		if (!PTR_ERR(dentry))
+			dput(dentry);
+		if (IS_ERR(new_parent))
+			return NULL;
+		if (old_parent)
+			dput(old_parent);
+		if (parent != *pp)
+			old_parent = parent;
+		parent = new_parent;
+	}
+	if (parent) {
+		*ref = 1;
+		*pp = parent;
+	}
+	if (old_parent)
+		dput(old_parent);
+	return strrchr(name, '/') + 1;
+}
+
 static int debugfs_create_by_name(const char *name, mode_t mode,
 				  struct dentry *parent,
 				  struct dentry **dentry,
@@ -153,6 +202,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 				  const struct file_operations *fops)
 {
 	int error = 0;
+	int ref = 0;
 
 	/* If the parent is not specified, we create it in the root.
 	 * We need the root dentry to do this, which is in the super 
@@ -161,6 +211,7 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 	 */
 	if (!parent)
 		parent = debugfs_mount->mnt_sb->s_root;
+	name = create_parents(name, parent, &parent, &ref);
 
 	*dentry = NULL;
 	mutex_lock(&parent->d_inode->i_mutex);
@@ -185,12 +236,17 @@ static int debugfs_create_by_name(const char *name, mode_t mode,
 		error = PTR_ERR(*dentry);
 	mutex_unlock(&parent->d_inode->i_mutex);
 
+	if (ref)
+		dput(parent);
+
 	return error;
 }
 
 /**
  * debugfs_create_file - create a file in the debugfs filesystem
  * @name: a pointer to a string containing the name of the file to create.
+ *        This can also contain directories (dir/file), in this case missing
+ *        directories will be automatically created.
  * @mode: the permission that the file should have.
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this paramater is NULL, then the
@@ -243,7 +299,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
 /**
  * debugfs_create_dir - create a directory in the debugfs filesystem
  * @name: a pointer to a string containing the name of the directory to
- *        create.
+ *        create. This can also contain directories (dir/file), in this case
+ *        missing directories will be automatically created.
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this paramater is NULL, then the
  *          directory will be created in the root of the debugfs filesystem.
@@ -269,7 +326,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
 /**
  * debugfs_create_symlink- create a symbolic link in the debugfs filesystem
  * @name: a pointer to a string containing the name of the symbolic link to
- *        create.
+ *        create.  This can also contain directories (dir/file), in this case
+ *        missing directories will be automatically created
  * @parent: a pointer to the parent dentry for this symbolic link.  This
  *          should be a directory dentry if set.  If this paramater is NULL,
  *          then the symbolic link will be created in the root of the debugfs
-- 
1.7.7.3

[PATCH 2/3] DEBUGFS: Add per cpu counters

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a simple way to export per cpu counters in debugfs.

This is done using a section, so that they can be declared using
a simple macro with minimal typing.

OPEN: modules are not supported yet.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/debugfs/Makefile               |    2 +-
 fs/debugfs/counter.c              |   73 +++++++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |    6 +++
 include/linux/debugfs.h           |   27 ++++++++++++++
 4 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 fs/debugfs/counter.c

diff --git a/fs/debugfs/Makefile b/fs/debugfs/Makefile
index 840c456..21be8d3 100644
--- a/fs/debugfs/Makefile
+++ b/fs/debugfs/Makefile
@@ -1,4 +1,4 @@
-debugfs-objs	:= inode.o file.o
+debugfs-objs	:= inode.o file.o counter.o
 
 obj-$(CONFIG_DEBUG_FS)	+= debugfs.o
 
diff --git a/fs/debugfs/counter.c b/fs/debugfs/counter.c
new file mode 100644
index 0000000..2372faf
--- /dev/null
+++ b/fs/debugfs/counter.c
@@ -0,0 +1,73 @@
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/percpu.h>
+#include <linux/kernel.h>
+
+/* OPEN: implement module support */
+
+extern struct debugfs_counter __start___debugfs[], __stop___debugfs[];
+
+static unsigned sum_counter(unsigned __percpu *counter)
+{
+	int cpu;
+	unsigned sum = 0;
+
+	for_each_online_cpu (cpu)
+		sum += per_cpu(*counter, cpu);
+	return sum;
+}
+
+static int
+dump_counters(struct seq_file *m,
+	      struct debugfs_counter *cnt, struct debugfs_counter *stop,
+	      char *fn)
+{
+	int n = 0;
+	for (; cnt < stop; cnt++)
+		if (fn[0] == cnt->fn[0] && !strcmp(cnt->fn, fn))
+			n += seq_printf(m, "%s: %u\n",
+					cnt->name, sum_counter(cnt->ptr));
+	return n;
+}
+
+static int show_debugfs_counter(struct seq_file *m, void *arg)
+{
+	int n;
+	n = dump_counters(m, __start___debugfs, __stop___debugfs, m->private);
+	return n;
+}
+
+static int debugfs_counter_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, show_debugfs_counter, inode->i_private);
+}
+
+static const struct file_operations debugfs_counter_fops = {
+	.open = debugfs_counter_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static void
+init_counters(struct debugfs_counter *cnt, struct debugfs_counter *stop)
+{
+	const char *last = NULL;
+
+	for (; cnt < stop; cnt++) {
+		if (cnt->fn == last)
+			continue;
+		last = cnt->fn;
+
+		/* Ignore error. Nothing we can do. */
+		debugfs_create_file(cnt->fn, 0444, NULL, (void *)cnt->fn,
+				    &debugfs_counter_fops);
+	}
+}
+
+static int init_debugfs_counters(void)
+{
+	init_counters(__start___debugfs, __stop___debugfs);
+	return 0;
+}
+module_init(init_debugfs_counters);
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..7a54092 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -272,6 +272,12 @@
 									\
 	TRACEDATA							\
 									\
+	__debugfs	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
+		VMLINUX_SYMBOL(__start___debugfs) = .;			\
+		*(__debugfs)						\
+		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
+	}								\
+									\
 	/* Kernel symbol table: Normal symbols */			\
 	__ksymtab         : AT(ADDR(__ksymtab) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start___ksymtab) = .;			\
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index e7d9b20..a243b05 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 
 #include <linux/types.h>
+#include <linux/percpu.h>
 
 struct file_operations;
 
@@ -76,6 +77,24 @@ struct dentry *debugfs_create_blob(const char *name, mode_t mode,
 
 bool debugfs_initialized(void);
 
+struct debugfs_counter {
+	unsigned __percpu *ptr;
+	const char *fn;
+	const char *name;
+} __attribute__((aligned(sizeof(char *))));
+
+/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
+#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
+	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
+	struct debugfs_counter name_ ## _pcpu_counter __used		\
+	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
+	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \
+
+#define DECLARE_DEBUGFS_COUNTER(name)			\
+	struct debugfs_counter name ## _pcpu_counter
+
+#define debugfs_counter_inc(name) this_cpu_inc(name ## _counter)
+
 #else
 
 #include <linux/err.h>
@@ -193,6 +212,14 @@ static inline bool debugfs_initialized(void)
 	return false;
 }
 
+
+struct debugfs_counter {
+};
+
+#define DEFINE_DEBUGFS_COUNTER(name, file)
+#define DECLARE_DEBUGFS_COUNTER(name)
+#define debugfs_counter_inc(name) do {} while(0)
+
 #endif
 
 #endif
-- 
1.7.7.3

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Thomas Gleixner]

On Tue, 13 Dec 2011, Andi Kleen wrote:
>  									\
> +	__debugfs	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
> +		VMLINUX_SYMBOL(__start___debugfs) = .;			\
> +		*(__debugfs)						\
> +		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
> +	}								\
> +									\

....
  
> +struct debugfs_counter {
> +	unsigned __percpu *ptr;
> +	const char *fn;
> +	const char *name;
> +} __attribute__((aligned(sizeof(char *))));
> +
> +/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
> +#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
> +	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
> +	struct debugfs_counter name_ ## _pcpu_counter __used		\
> +	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
> +	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \

Sigh, we had that section forms an array problem more than once
already. Why do you invent another variant and think that it will not
explode?

Your alignment magic does not guarantee at all that the structs will
form an array. The "aligned" attribute guarantees only the _MINIMUM_
alignment for a structure, but the compiler and the linker are free to
align on larger multiples.

See commit 654986462 for details. That has been discussed to death and
your solution wont work either across all the various compiler
incarnations we have and will have.

Thanks,

	tglx

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Andi Kleen]

On Tue, Dec 13, 2011 at 11:43:16PM +0100, Thomas Gleixner wrote:
> On Tue, 13 Dec 2011, Andi Kleen wrote:
> >  									\
> > +	__debugfs	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
> > +		VMLINUX_SYMBOL(__start___debugfs) = .;			\
> > +		*(__debugfs)						\
> > +		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
> > +	}								\
> > +									\
> 
> ....
>   
> > +struct debugfs_counter {
> > +	unsigned __percpu *ptr;
> > +	const char *fn;
> > +	const char *name;
> > +} __attribute__((aligned(sizeof(char *))));
> > +
> > +/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
> > +#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
> > +	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
> > +	struct debugfs_counter name_ ## _pcpu_counter __used		\
> > +	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
> > +	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \
> 
> Sigh, we had that section forms an array problem more than once
> already. Why do you invent another variant and think that it will not
> explode?

I did three or four different sections like this in the past and as far
as I know none of them has exploded so far in production use.

Can you be more specific? Where exactly do you think this will
not work?

> Your alignment magic does not guarantee at all that the structs will
> form an array. The "aligned" attribute guarantees only the _MINIMUM_
> alignment for a structure, but the compiler and the linker are free to
> align on larger multiples.

> 
> See commit 654986462 for details.

Doesn't give a lot of details actually. Which target?

Note that my structure only has pointers, so there is not a lot 
of potential for "evil" alignment.

-Andi

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Steven Rostedt]

On Tue, 2011-12-13 at 14:56 -0800, Andi Kleen wrote:
>  
> > > +struct debugfs_counter {
> > > +	unsigned __percpu *ptr;
> > > +	const char *fn;
> > > +	const char *name;
> > > +} __attribute__((aligned(sizeof(char *))));
> > > +

> > 
> > See commit 654986462 for details.
> 
> Doesn't give a lot of details actually. Which target?
> 
> Note that my structure only has pointers, so there is not a lot 
> of potential for "evil" alignment.

We hit this with trace events as well, and your structure does have an
evil alignment, it's 3 pointers, which is not a base 2 number. Thus,
there were some compilers that liked to add padding to make the
alignment a power of 2. That is, between two sections of files we ended
up with something like:


.section file1
.ptra1
.ptra2
.ptra3
.ptrb1
.ptrb2
.ptrb3
.ptrc1
.ptrc2
.ptrc3
.ptrd1
.ptrd2
.ptrd3
.ptre1
.ptre2
.ptre3
<space>
.section file2
.ptrf1
.ptrf2
.ptrf3
[...]



It didn't happen often, heck, trace events and tracepoints were like
this for sometime before this blew up in our faces.

>From what I've seen is that 1 or 2 longs will pack nicely, but anything
else (except for maybe 4, 8, 16, etc) will run a risk of crashing.

-- Steve

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2011-12-13 at 14:56 -0800, Andi Kleen wrote:
> >  
> > > > +struct debugfs_counter {
> > > > +	unsigned __percpu *ptr;
> > > > +	const char *fn;
> > > > +	const char *name;
> > > > +} __attribute__((aligned(sizeof(char *))));
> > > > +
> 
> > > 
> > > See commit 654986462 for details.
> > 
> > Doesn't give a lot of details actually. Which target?
> > 
> > Note that my structure only has pointers, so there is not a lot 
> > of potential for "evil" alignment.
> 
> We hit this with trace events as well, and your structure does have an
> evil alignment, it's 3 pointers, which is not a base 2 number. Thus,
> there were some compilers that liked to add padding to make the
> alignment a power of 2. That is, between two sections of files we ended
> up with something like:
> 
> 
> .section file1
> .ptra1
> .ptra2
> .ptra3
> .ptrb1
> .ptrb2
> .ptrb3
> .ptrc1
> .ptrc2
> .ptrc3
> .ptrd1
> .ptrd2
> .ptrd3
> .ptre1
> .ptre2
> .ptre3
> <space>
> .section file2
> .ptrf1
> .ptrf2
> .ptrf3
> [...]
> 
> 
> 
> It didn't happen often, heck, trace events and tracepoints were like
> this for sometime before this blew up in our faces.
> 
> From what I've seen is that 1 or 2 longs will pack nicely, but anything
> else (except for maybe 4, 8, 16, etc) will run a risk of crashing.

Yep. We changed our implementations to use an array of pointers to the
elements rather than creating a section of these elements per se to make
sure we would not trigger this problem anymore. Unless my memory fails
me, it started blowing up in our faces with new gcc versions. The thing
is that gcc is within its right to happily over-align those structures.
__attribute__((aligned())) is just a hint providing a minimum alignment
to respect, but the compiler can choose a higher alignment value as it
sees fit.

Mathieu

> 
> -- Steve
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Steven Rostedt]

On Tue, 2011-12-13 at 18:08 -0500, Steven Rostedt wrote:

> We hit this with trace events as well, 

Here's the commit if you were interested:

e4a9ea5ee7c8812a7bf0c3fb725ceeaa3d4c2fcc

-- Steve

Re: [PATCH 2/3] DEBUGFS: Add per cpu counters

[Thomas Gleixner]

On Tue, 13 Dec 2011, Andi Kleen wrote:
> On Tue, Dec 13, 2011 at 11:43:16PM +0100, Thomas Gleixner wrote:
> > On Tue, 13 Dec 2011, Andi Kleen wrote:
> > >  									\
> > > +	__debugfs	: AT(ADDR(__debugfs) - LOAD_OFFSET) {		\
> > > +		VMLINUX_SYMBOL(__start___debugfs) = .;			\
> > > +		*(__debugfs)						\
> > > +		VMLINUX_SYMBOL(__stop___debugfs) = .;			\
> > > +	}								\
> > > +									\
> > 
> > ....
> >   
> > > +struct debugfs_counter {
> > > +	unsigned __percpu *ptr;
> > > +	const char *fn;
> > > +	const char *name;
> > > +} __attribute__((aligned(sizeof(char *))));
> > > +
> > > +/* Note: static doesn't work unlike DEFINE_PERCPU. Sorry. */
> > > +#define DEFINE_DEBUGFS_COUNTER(name_, file)				\
> > > +	DEFINE_PER_CPU(unsigned, name_ ## _counter);			\
> > > +	struct debugfs_counter name_ ## _pcpu_counter __used		\
> > > +	__attribute__((aligned(sizeof(char *)),section("__debugfs"),unused)) \
> > > +	= { .ptr = &name_ ## _counter, .fn = file, .name = #name_ }; \
> > 
> > Sigh, we had that section forms an array problem more than once
> > already. Why do you invent another variant and think that it will not
> > explode?
> 
> I did three or four different sections like this in the past and as far
> as I know none of them has exploded so far in production use.
> 
> Can you be more specific? Where exactly do you think this will
> not work?

Can you actually read and understand what I wrote below?
 
> > Your alignment magic does not guarantee at all that the structs will
> > form an array. The "aligned" attribute guarantees only the _MINIMUM_
> > alignment for a structure, but the compiler and the linker are free to
> > align on larger multiples.
> 
> > 
> > See commit 654986462 for details.
> 
> Doesn't give a lot of details actually.

There are enough details to figure it out. http://www.jfgit.com might
help.

>  Which target?

The target is irrelevant.

> Note that my structure only has pointers, so there is not a lot 
> of potential for "evil" alignment.

That does NOT matter at ALL. Also whether you did that in the past or
not is completely irrelevant. Its also totally irrelevant whether your
structure only has pointers or not.

There is NO GUARANTEE that this will end up in a linear array with a
stride of the struct size. Period.

Just get it.

Thanks,

	tglx