Scalable statistics counters using seq_file interfaces
PATCH 1/1 Scalable statistics counters using seq_file interfaces
Hi Andrew,
Here is the statctr 2.5.31 patch which uses the seq_file interfaces for /proc reporting now. Do you think it is good enough to get into mainline kernel now? The foll patch depends on kmalloc_percpu patch for 2.5.31 published earlier.
Christoph, since statctr_pentry describes only a proc entry for a set of counters, I thought it'd be more descriptive to use statctr_proc_entry or statctr_pentry...something to indicate it is for proc reporting only. If CONFIG_PROC_FS is not defined, statctr_pentry functions will not be available. This was my line of thought, of course I can change it to statctr_group if you still feel that is better.
Thanx for the review and suggestions.
Thanks,
Kiran
fs/proc/root.c +2 -0
@@ -19,6 +19,7 @@#include <linux/smp_lock.h> struct proc_dir_entry *proc_net, *proc_bus, *proc_root_fs, *proc_root_driver; +struct proc_dir_entry *proc_statistics;#ifdef CONFIG_SYSCTL struct proc_dir_entry *proc_sys_root;
@@ -77,6 +78,7 @@proc_rtas_init ();#endif proc_bus =proc_mkdir ("bus" ,0 ); + proc_statistics =proc_mkdir ("statistics" ,0 ); }static struct dentry *proc_root_lookup (struct inode * dir,struct dentry * dentry)
include/linux/statctr.h +254 -0
254 lines not on this page · raw message
kernel/Makefile +3 -2
@@ -10,12 +10,13 @@ O_TARGET := kernel.o export-objs = signal.o sys.o kmod.o context.o ksyms.o pm.o exec_domain.o \ - printk.o platform.o suspend.o dma.o + printk.o platform.o suspend.o dma.o statctr.o obj-y = sched.o fork.o exec_domain.o panic.o printk.o \ module.o exit.o itimer.o time.o softirq.o resource.o \ sysctl.o capability.o ptrace.o timer.o user.o \ - signal.o sys.o kmod.o context.o futex.o platform.o + signal.o sys.o kmod.o context.o futex.o platform.o \ + statctr.o obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o obj-$(CONFIG_SMP) += cpu.o
kernel/statctr.c +264 -0
264 lines not on this page · raw message
Quoted 4 lines · Ravikiran G Thirumalai
Christoph, since
statctr_pentrydescribes only a proc entry for a set of counters, I thought it'd be more descriptive to usestatctr_proc_entryor statctr_pentry...something to indicate it is for proc reporting only.
My educated guess is that I will rewrite the code to not depend on procfs before 2.6 is out.. But as you use other procfs types it's just one more search and replace, so..
Quoted 12 lines
diff -ruN -X dontdiff
linux-2.5.31/fs/proc/root.cstatctr-2.5.31/fs/proc/root.cfs/proc/root.c +1
-0@@ -19,6 +19,7 @@#include <linux/smp_lock.h> struct proc_dir_entry *proc_net, *proc_bus, *proc_root_fs, *proc_root_driver; +struct proc_dir_entry *proc_statistics;#ifdef CONFIG_SYSCTL struct proc_dir_entry *proc_sys_root;
fs/proc/root.c line 81
Quoted 4 lines
fs/proc/root.c +1
-0@@ -77,6 +78,7 @@proc_rtas_init ();#endif proc_bus =proc_mkdir ("bus" ,0 ); + proc_statistics =proc_mkdir ("statistics" ,0 );
Any reason you don't do this in kernel/statctr.c?
fs/proc/root.c
Quoted 4 lines
fs/proc/root.c +4
-0+#ifndef _LINUX_STATCTR_H +#define _LINUX_STATCTR_H + +#ifdef __KERNEL__
Is this needed?
fs/proc/root.c
Quoted 2 lines
fs/proc/root.c +2
-0+static inline struct statctr_pentry +*create_statctr_pentry (struct proc_dir_entry *parent,const char *name)
Shouldn't this be:
static inline struct statctr_pentry * create_statctr_pentry(struct proc_dir_entry *parent, const char *name)
fs/proc/root.c
Quoted 3 lines
fs/proc/root.c +3
-0+{ +return (NULL ); +}
return is not function-like and the additional braces are heavily disliked in the kernel (at least for new code)
fs/proc/root.c
Quoted 4 lines
fs/proc/root.c +4
-0+static inline int __statctr_init (statctr_t *stctr,int gfp_mask) +{ + stctr->ctr =kmalloc_percpu (sizeof (*(stctr->ctr)), gfp_mask); +if (!stctr->ctr)
Kernel coding style has a space after the if.
fs/proc/root.c
Quoted 1 line
fs/proc/root.c +1
-0+return -1 ;
-ENOMEM?
fs/proc/root.c
Quoted 3 lines
fs/proc/root.c +3
-0+void free_statctr_pentry (struct statctr_pentry *pentry) +{ +if (!list_empty (&pentry->head))
Add an unlikely?
It might be worth to rework the code to follow Documentation/CodingStyle
Hello Christoph,
Quoted 2 lines · Christoph Hellwig
...
My educated guess is that I will rewrite the code to not depend on procfs
Right now you have procfs dependency only if you need /proc reporting. You can have satctrs without /proc too, you just have to pass NULL to statctr_group parameter in statctr_init ......
Quoted 9 lines
Quoted 9 lines
diff -ruN -X dontdiff
linux-2.5.31/fs/proc/root.cstatctr-2.5.31/fs/proc/root.cfs/proc/root.c +1
-0@@ -19,6 +19,7 @@#include <linux/smp_lock.h> struct proc_dir_entry *proc_net, *proc_bus, *proc_root_fs, *proc_root_driver; +struct proc_dir_entry *proc_statistics;
Quoted 9 lines
fs/proc/root.c line 81
Quoted 7 lines
fs/proc/root.c +1
-0#ifdef CONFIG_SYSCTL struct proc_dir_entry *proc_sys_root;@@ -77,6 +78,7 @@proc_rtas_init ();#endif proc_bus =proc_mkdir ("bus" ,0 ); + proc_statistics =proc_mkdir ("statistics" ,0 );Any reason you don't do this in
kernel/statctr.c?
I don't have an init routine at boot time to create the default parent directory for statctr proc entries; Of course, I can make one and put it in statctr.c and call it from init/main.c, but I thought this must be better than that.
Quoted 7 lines
fs/proc/root.c
Quoted 5 lines
fs/proc/root.c +4
-0+#ifndef _LINUX_STATCTR_H +#define _LINUX_STATCTR_H + +#ifdef __KERNEL__Is this needed?
I have seen this check used frequently. I assumed it was to prevent userspace from using the headers....hence the ifdefs. Pls let me know if "#ifdef __KERNEL__" has any other reasoning behind it in other places where it is used right now.... anywayz that's gone now.
Quoted 8 lines
fs/proc/root.c
Quoted 3 lines
fs/proc/root.c +2
-0+static inline struct statctr_pentry +*create_statctr_pentry (struct proc_dir_entry *parent,const char *name)Shouldn't this be:
static inline struct
statctr_pentry*create_statctr_pentry(struct proc_dir_entry *parent, const char *name)
Sure, I can change that, but there is no documented preference on that AFAIK.
Quoted 7 lines
fs/proc/root.c
Quoted 4 lines
fs/proc/root.c +3
-0+{ +return (NULL ); +}return is not function-like and the additional braces are heavily disliked in the kernel (at least for new code)
I should have used a consistent format for return at least... I'll change this too, but again it is not documented in CodingStyle.
Quoted 8 lines
fs/proc/root.c
Quoted 5 lines
fs/proc/root.c +4
-0+static inline int __statctr_init (statctr_t *stctr,int gfp_mask) +{ + stctr->ctr =kmalloc_percpu (sizeof (*(stctr->ctr)), gfp_mask); +if (!stctr->ctr)Kernel coding style has a space after the if.
There is no explicit in CodingStyle on the space after if, but that is part of the K&R coding style I guess.....
Quoted 3 lines
fs/proc/root.c
Quoted 1 line
fs/proc/root.c +1
-0+return -1 ;
-ENOMEM?
Yep, that'd be better.
Quoted 6 lines
fs/proc/root.c
Quoted 4 lines
fs/proc/root.c +3
-0+void free_statctr_pentry (struct statctr_pentry *pentry) +{ +if (!list_empty (&pentry->head))Add an unlikely?
Sure, but does it really make any difference? (It ain't fastpath ... you probably won't call this routine at all .. maybe at subsystem end or module unload...)
Quoted 2 lines
It might be worth to rework the code to follow
Documentation/CodingStyle
I had read the CodingStyle sometime back, and I thought I conformed to it, Most of the stuff you've pointed out are not in CodingStyle right now, perhaps its time to update Documentation/CodingStyle?
Thanks for the feedback, Here's the new patch .. like it?
-Kiran
fs/proc/root.c +2 -0
@@ -19,6 +19,7 @@#include <linux/smp_lock.h> struct proc_dir_entry *proc_net, *proc_bus, *proc_root_fs, *proc_root_driver; +struct proc_dir_entry *proc_statistics;#ifdef CONFIG_SYSCTL struct proc_dir_entry *proc_sys_root;
@@ -77,6 +78,7 @@proc_rtas_init ();#endif proc_bus =proc_mkdir ("bus" ,0 ); + proc_statistics =proc_mkdir ("statistics" ,0 ); }static struct dentry *proc_root_lookup (struct inode * dir,struct dentry * dentry)
include/linux/statctr.h +245 -0
245 lines not on this page · raw message
kernel/Makefile +3 -2
@@ -10,12 +10,13 @@ O_TARGET := kernel.o export-objs = signal.o sys.o kmod.o context.o ksyms.o pm.o exec_domain.o \ - printk.o platform.o suspend.o dma.o + printk.o platform.o suspend.o dma.o statctr.o obj-y = sched.o fork.o exec_domain.o panic.o printk.o \ module.o exit.o itimer.o time.o softirq.o resource.o \ sysctl.o capability.o ptrace.o timer.o user.o \ - signal.o sys.o kmod.o context.o futex.o platform.o + signal.o sys.o kmod.o context.o futex.o platform.o \ + statctr.o obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o obj-$(CONFIG_SMP) += cpu.o
kernel/statctr.c +265 -0
265 lines not on this page · raw message