On 5/27/22 14:52, Jason Gunthorpe wrote:
On Wed, May 25, 2022 at 08:50:52PM +0200, Bart Van Assche wrote:
On 5/25/22 13:01, Sagi Grimberg wrote:
iirc this was reported before, based on my analysis lockdep is giving
a false alarm here. The reason is that the id_priv->handler_mutex cannot
be the same for both cm_id that is handling the connect and the cm_id
that is handling the rdma_destroy_id because rdma_destroy_id call
is always called on a already disconnected cm_id, so this deadlock
lockdep is complaining about cannot happen.
I'm not sure how to settle this.
If the above is correct, using lockdep_register_key() for
id_priv->handler_mutex instead of a static key should make the lockdep false
positive disappear.
That only works if you can detect actual different lock classes during
lock creation. It doesn't seem applicable in this case.
Why doesn't it seem applicable in this case? The default behavior of
mutex_init() and related initialization functions is to create one lock
class per synchronization object initialization caller.
lockdep_register_key() can be used to create one lock class per
synchronization object instance. I introduced lockdep_register_key()
myself a few years ago.
After having taken a closer look at the RDMA/CM code, I decided not yet
to implement what I proposed above. I noticed that handler_mutex is held
around callback invocations. An example:
static int cma_cm_event_handler(struct rdma_id_private *id_priv,
struct rdma_cm_event *event)
{
int ret;
lockdep_assert_held(&id_priv->handler_mutex);
trace_cm_event_handler(id_priv, event);
ret = id_priv->id.event_handler(&id_priv->id, event);
trace_cm_event_done(id_priv, event, ret);
return ret;
}
My opinion is that holding *any* lock around the invocation of a
callback function is an antipattern, in other words, something that
never should be done. John Ousterhout already described this in 1996 in
his presentation [1]. Patches like 071ba4cc559d ("RDMA: Add
rdma_connect_locked()") work around this problem but do not solve it.
Has it been considered to rework the RDMA/CM such that no locks are held
around the invocation of callback functions like the event_handler
callback? There are other mechanisms to report events from one software
layer (RDMA/CM) to a higher software layer (ULP), e.g. a linked list
with event information. The RDMA/CM could queue events onto that list
and the ULP can dequeue events from that list.
Thanks,
Bart.
[1] Ousterhout, John. "Why threads are a bad idea (for most purposes)."
In Presentation given at the 1996 Usenix Annual Technical Conference,
vol. 5. 1996.
