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

[Christoph Hellwig]

On Tue, Jun 14, 2005 at 07:09:16PM -0700, Caitlin Bestler wrote:
> These are attributes that are required for all sub-types that comply
> with a parent type. The original code makes the every so slight
> mistake of including the IA in that list of types, which has the
> horrendous defect of having an IA pointer in the IA itself. 
> 
> It would be guaranteed to drive a Java programmer insane,
> but most C programmers would figure "How many IA objects
> are there anyway. what's the harm in having one meaningless
> field in a type that there are only a handful of instances of?
> I'll declare one fewer type and "waste" the pointer space,
> I'll probably make it up in code space."
> 
> Are you implying that implementing the same requirement
> multiple time as though it were not a common requirement
> somehow makes the code "simpler"?

If there actually were common requirements.  As Mentioned only a
partially-used linked list and a questionable synchronization object are
common.  Anyway, I think discussing this question here is rather mood
because dapl_common will gradually go away anyway, the list should move
to the dat layer and the lock for those objects where it makes sense
aswell.

> There actually is a solution to that in DAT. It allows the DAT Consumer
> to hold the lock rather than individual DAT objects. It is an option that
> I would actually expect *most* kernel clients to prefer.
> 
> That's the meaning of the "thread safety" flag, it documents *who*
> is responsible for making access to each DAT object thread safe:
> the DAT Provider or the DAT Consumer. 
> 
> Doing an unnecessary spinlock, particularly in an SMP kernel, is
> a major destroyer of efficiency. That is why DAT was designed
> so that the Consumer could load a "thread optimized" library
> (where it was responsible for serializing access to each object)
> or a "thread safe" library where the DAT Provider would do so.
> 
> But even when the latter is needed the impact of a spinlock
> is lessened when the scope of the lock is no wider than needed.
> Fastpath operations such as dat_ep_post_send should only 
> lock its own send queue. Locking some other object, like the
> IA, would cause unneeded blockages. With the lock being
> on the scale of the EP, 4 different processors can be concurrently
> posting to four different EPs without interference. If you "safe"
> that lock by moving it to the IA you'll limit the system to doing
> one post at a time no matter how many processor are available.

I completely agree on your assertation on locking, it's the direction
I was thinging about when saying one lock per object is a bad idea.
Once the code is cleaned up a little and more understandable we should
design a sane locking scheme, and that will involve a lock in the
dat_ia structure at least that can be held over multiple operations.