Re: [Tipc-discussion] Re: spinlock deadlock

[Jon M. <jon...@er...>]

See below.
/jon

Daniel McNeil wrote:


On Thu, 2004-05-13 at 08:10, Mark Haverkamp wrote:

  

On Wed, 2004-05-12 at 13:25, Mark Haverkamp wrote:

    

On Wed, 2004-05-12 at 12:09, Jon Maloy wrote:

      

Yeah, that's a classical one. I also think your solution is ok; the

pending ports will be awakened at next message reception, so

no harm done.



Thanks /jon



        

I found another one.  I got another hang yesterday after the current

deadlock fix.  I re-added my spinlock debug code and found out that

we're getting a deadlock between the node lock and the tipc_port lock.

It looks like the port timeout handler is running on one CPU and a

recv_msg is running on the other.  I suppose as a workaround, we could

make all the spin lock access in wakeup be conditional, but that will

probably just make the problem just show up somewhere else.  There

should probably be an analysis of code paths and determine how the locks

interact with each other.  

    



I agree.  The locking hierarchy should be documented.  Even if this

is just comments a source file, it needs to be done.  It should

also include exactly what the lock is protecting.

Agree completely. I wil try to do my part.








  

I have noticed that there is at least one

place where three locks are required.  This can cause problems like

we've seen when different code paths need multiple locks unless there is

some sort of back off method to insure no deadlocks.  



    



We also need to analyze if the multiple locks are actually giving us

any performance and/or parallelism benefits.  If we have to take

multiple locks in a common path, that might be causing worse performance

and more deadlock potential.

We could experiment on that. The one-big-lock approach that we had
eariler
was heavily critisized, and caused a lot of problems, but so far
measurements
suggest it was mor efficient when running the benchmark between two
nodes.

When we start to run real systems I am pretty convinced that the 
finer granularity will pay off, as we will have thousands of ports and 
dozens of nodes being accessed in parallel.

In the previous version we had seven locks/unlocks per message
transmitted:

Send side: 1x  big_lock/unlock when message was created and sent, 2x
buffer locks
                  when buffer was allocated and released. 
Receive side: Ditto + 1x  grab/release of big_lock when message is read
to
                  user space.

I don't think it is possible to make it much better.


In the current version we have ten:

Send side:  1x net_lock  (read) + 1x node lock + 2 x buffer lock. (No
tipc_port lock!)
Receive side: 1 net_read_lock + 1x node lock + 1x tipc_port_lock + 1x
tipc_port_lock
                        when message is read (in "advance_queue()" + 2x
buffer lock).

I don't think an increase from seven to ten lock accesses adds any
significant 
overhead to the data-phase path, while the advantages are obvious.

The real problem is not performance, but potential deadlock situations
in
other parts of the code. Those can only be solved by as far as possible 
avoiding to keep the different locks simultaneously, as I suggested in
my 
previous mail. There may be one two cases more I have missed, but 
if we find any, this is the solution we  should look for.

Regards /Jon










Daniel