Re: [patch] Scalable statistics counters using seq_file interfaces

Christoph Hellwig (hch@infradead.org)
Wed, 14 Aug 2002 19:37:02 +0100


On Wed, Aug 14, 2002 at 04:50:49PM +0530, Ravikiran G Thirumalai wrote:
> 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.

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..

> diff -ruN -X dontdiff linux-2.5.31/fs/proc/root.c statctr-2.5.31/fs/proc/root.c
> --- linux-2.5.31/fs/proc/root.c Sun Aug 11 07:11:50 2002
> +++ statctr-2.5.31/fs/proc/root.c Tue Aug 13 10:14:46 2002
> @@ -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);

Any reason you don't do this in kernel/statctr.c?

> +#ifndef _LINUX_STATCTR_H
> +#define _LINUX_STATCTR_H
> +
> +#ifdef __KERNEL__

Is this needed?

> +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)

> +{
> + return(NULL);
> +}

return is not function-like and the additional braces are heavily disliked
in the kernel (at least for new code)

> +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.

> + return -1;

-ENOMEM?

> +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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/