Re: [PATCH] lock stat for -rt 2.6.20-rc2-rt2.2.lock_stat.patch

[Ingo Molnar]

* Bill Huey <billh@xxxxxxxxxxxxxxxxx> wrote:

> >  - Documentation/CodingStyle compliance - the code is not ugly per se
> >    but still looks a bit 'alien' - please try to make it look Linuxish,
> >    if i apply this we'll probably stick with it forever. This is the
> >    major reason i havent applied it yet.
> 
> I reformatted most of the patch to be 80 column limited. I simplified 
> a number of names, but I'm open to suggestions and patches to how to 
> go about this. Much of this code was a style experiment, but now I 
> have to make this more mergable.

thanks. It's looking better, but there's still quite a bit of work left:

there's considerable amount of whitespace noise in it - lots of lines 
with space/tab at the end, lines with 8 spaces instead of tabs, etc.

comment style issues:

+/* To be use for avoiding the dynamic attachment of spinlocks at runtime
+ * by attaching it inline with the lock initialization function */

the proper multi-line style is:

/*
 * To be used for avoiding the dynamic attachment of spinlocks at 
 * runtime by attaching it inline with the lock initialization function:
 */

(note i also fixed a typo in the one above)

more unused code:

+/*
+static DEFINE_LS_ENTRY(__pte_alloc);
+static DEFINE_LS_ENTRY(get_empty_filp);
+static DEFINE_LS_ENTRY(init_waitqueue_head);
...
+*/

these:

+static int lock_stat_inited = 0;

should not be initialized to 0, that is implicit for static variables.

weird alignment here:

+void lock_stat_init(struct lock_stat *oref)
+{
+       oref->function[0]       = 0;
+       oref->file      = NULL;
+       oref->line      = 0;
+
+       oref->ntracked  = 0;

funky branching:

+       spin_lock_irqsave(&free_store_lock, flags);
+       if (!list_empty(&lock_stat_free_store)) {
+               struct list_head *e = lock_stat_free_store.next;
+               struct lock_stat *s;
+
+               s = container_of(e, struct lock_stat, list_head);
+               list_del(e);
+
+               spin_unlock_irqrestore(&free_store_lock, flags);
+
+               return s;
+       }
+       spin_unlock_irqrestore(&free_store_lock, flags);
+
+       return NULL;

that should be s = NULL in the function scope and a plain unlock and 
return s.

assignments mixed with arithmetics:

+static
+int lock_stat_compare_objs(struct lock_stat *x, struct lock_stat *y)
+{
+       int a = 0, b = 0, c = 0;
+
+       (a = ksym_strcmp(x->function, y->function))     ||
+       (b = ksym_strcmp(x->file, y->file))             ||
+       (c = (x->line - y->line));
+
+       return a | b | c;

the usual (and more readable) style is to separate them out explicitly:

	a = ksym_strcmp(x->function, y->function);
	if (!a)
		return 0;
	b = ksym_strcmp(x->file, y->file);
	if (!b)
		return 0;

	return x->line == y->line;

(detail: this btw also fixes a bug in the function above AFAICS, in the 
a && !b case.)

also, i'm not fully convinced we want that x->function as a string. That 
makes comparisons alot slower. Why not make it a void *, and resolve to 
the name via kallsyms only when printing it in /proc, like lockdep does 
it?

no need to put dates into comments:

+        * Fri Oct 27 00:26:08 PDT 2006

then:

+       while (node)
+       {

proper style is:

+	while (node) {

this function definition:

+static
+void lock_stat_insert_object(struct lock_stat *o)

can be single-line. We make it multi-line only when needed.

these are only samples of the types of style problems still present in 
the code.

	Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/