[openib-general] Re: [PATCH] kDAPL: convert the ep list to linux native

[Christoph Hellwig]

On Tue, Jun 14, 2005 at 05:20:39PM -0400, James Lentini wrote:
> >Purely a style issue.
> 
> Is it a personal style issue or a Linux kernel style issue?

It's a personal interpretation of the Linux style rule "don't make
things complicated if they can be done simple and less confusing".

Look at the structure, there's:

  struct dapl_ia *owner_ia;

this insn't actually usefull in all users of dapl_common, and it's easy
enough to use without it.  No polymorphism argument either because you
could just pass ->owner_ia to whatever function expects it.

  spinlock_t lock;

now in every subsystem I looked at putting a lock into every single
object was a mistake.  You end up with a something as slow and
undebuggable as Solaris if you do that.  Note sure if it's true for the
current kDAPL codebase, but in the end there shouldn't be a lock needed
in every structure.

  unsigned long flags;              /* saved lock flag values */

now this one is completely wrong.  the irqflags must always be restored
in the same function they're used in, and the canoncical way to do that
is to put them onto the stack, as everyone in the kernel does.  putting
them in some object on the heap is a total wate of memory and encourages
using the locks at different levels which is wrong.