[ofa-general] Re: [PATCH 2.6.24] rdma/cm: fix deadlock destroying listen requests

[Kanoj Sarcar]

Sean Hefty wrote:

>> Just so I understand, did you discover problems (maybe preexisting 
>> race conditions) with my previously posted patch? If yes, please 
>> point it out, so its easier to review yours; if not, I will assume 
>> your patch implements a better locking scheme and review it as such.
>
>

Sean,

I looked over your patch for a while.

Agreed, your patch fixes a race condition that my patch had exposed (I 
had analyzed the sequence wildcard destruct getting to a device listener 
before a racing device removal could, but not the reverse order).

I do have some issues though:

* in your patch, I suggest taking out the warning printk from 
cma_listen_on_dev() when the listener create attempt fails; it might be 
that the device is out of resources etc. Since the code takes care of 
this situation pretty well, I don't see a need for the printk.

* I don't see a reason for the internal_id and the device listeners 
getting a refcount on the wildcard listener. Because, even without 
these, it is guaranteed that the wildcard listener will exist at least 
as long as any of the children device listener's are around, by looking 
at the logic in rdma_destroy_id(). Can you provide some logic for 
requring this then?

* not that I am very worried (and I suggesting resolving this thru 
another subsequent patch if it is really a problem), but I think device 
removal is still racy wrt non wildcard listeners. Here's the sequence: 
cma_process_remove()->cma_remove_id_dev() decides it will 
rdma_destroy_id() the listener id, and at the same time a process 
context rdma_destroy_id() decides it is going to do the same. There are 
probably various ways to take care of this, the simple one might be for 
rdma_destroy_id() to look at the "state" and make a decision about who 
gets to destroy.

Thanks.

Kanoj

> I tried to explain the issue somewhat in my change commit and code 
> comments.  The issue is synchronizing cleanup of the listen_list with 
> device removal.
>
> When an RDMA device is added to the system, a new listen request is 
> added for all wildcard listens.  Since the original locking held the 
> mutex throughout the cleanup of the listen list, it prevented adding 
> another listen request during that same time.
>
> Similar protection was there for handling device removal.  When a 
> device is removed from the system, all internal listen requests 
> associated with that device are destroyed.  If the associated wildcard 
> listen is also being destroyed, we need to ensure that we don't try to 
> destroy the same listen twice.
>
> My patch, like yours, ends up releasing the mutex while cleaning up 
> the listen_list.  I choose to eliminate the cma_destroy_listen() call, 
> and use rdma_destroy_id() as a single destruction path instead.  This 
> keeps the locking contained to a single function.  (I don't like 
> acquiring a lock in one call and releasing it in another.  It puts too 
> much assumption on the caller.)
>
> What was missing was ensuring that a device removal didn't try to 
> destroy the same listen request.  This is handled by the adding the 
> list_del*() calls to cma_cancel_listens().  Whichever thread removes 
> the listening id from the device list is responsible for its 
> destruction. And because that thread could be the device removal 
> thread, I added a reference from the per device listen to the wildcard 
> listen.
>
> Hopefully this makes sense.
>
> - Sean
>