On Thu, Apr 16, 2015 at 07:31:22PM +0200, David Herrmann wrote:
> I'm working on patches to add more comments similar to how we did in
> node.c. For now, please see my explanations below:
>
> node->lock is the _innermost_ lock.
> node->active implements revoke
> support for nodes. It follows what kernfs->active does and isn't a
> lock in particular. We kinda treat it as rwsem, where down_write() is
> the outer-most lock in kdbus and _only_ called without any other lock
> held (kdbus_node_deactivate()). Read-side, we never ever block on the
> "lock", but only use try-lock. If it fails, the node is dead/revoked.
> Therefore, the read-side of 'active' nests almost arbitrarily. We hold
> 'active'-references almost everywhere, to make sure a node is not
> destroyed while we use it. However, we never sleep for an indefinite
> time while holding it.
Umm... Theoretically, but ->mmap_sem being under it means that it might
involve something like an NFS server timing out, so the latency might
suck very badly.
> Given that the write-side is the outer-most lock in kdbus, it doesn't
> dead-lock against the try-lock readers.
Huh? I see at least this call chain:
kdbus_handle_ioctl_control()
kdbus_node_acquire()
kdbus_cmd_bus_make()
kdbus_node_deactivate()
Granted, it won't be the _same_ node (otherwise you'd deadlock solid
right there and then), but it means that your locking order is sensitive
to something about nodes; it's not entirely determined by the lock type.
> Locking order (outer-most to inner-most):
> 1) domain->lock
> 2) names->rwlock
> 3) endpoint->lock
> 4) bus->conn_rwlock
> 5) policy->entries_rwlock
> 6) connection->lock
> 7) metadata->lock
>
> mmap_sem nests below metadata->lock. With the rcu-protected exe_file
> patches by Davidlohr Bueso, we can even drop that dependency. They
> have kinda stalled, though.
>
> Then we have a bunch of data structure protection, which can be called
> from any context:
> * bus->notify_lock
> * pool->lock
> * match->mdb_rwlock
> * node->lock
>
> Lastly, there're 2 locks which nest around everything and must not be
> taken with any lock held:
> * handle->rwlock (taken in ioctl-entry)
as well as in ->poll(), for completeness sake. The latter, BTW, isn't
nice - kdbus is far from being the only thing that does it, but having
->poll() block can be somewhat surprising...
> * bus->notify_flush_lock (taken in work-queue)
Hmm... That needs some care - it means that it nests inside anything held
by callers of cancel_delayed_work_sync() on the corresponding work. AFAICS,
there's at least one call chain leading to that from kdbus_node_deactivate()
(via ->release_cb == kdbus_ep_release -> kdbus_conn_disconnect ->
cancel_delayed_work_sync(&conn->work)) wait for kdbus_reply_list_scan_work
-> kdbus_notify_flush grabs ->notify_flush_lock). Tracking back further is
harder - not all call sites of kdbus_node_deactivate() can lead to that...
BTW, it's not only done in wq callbacks - there's a direct chain from
kdbus_conn_disconnect() as well (both through kdbus_name_release_all ->
kdbus_notify_flush and directly through kdbus_notify_flush()). And from
ioctl(), by many paths, while we are at it, but that only means that it
nests inside handle->rwlock, and _that_ is really the outermost.
What nests inside that one? It definitely a part of hierarchy - it can't
be excluded from deadlock analysis as effectively outermost. As for the
stuff under it... registry->rwlock is obvious, what else?
> General object stacking is:
> domain -> bus -> endpoint -> policy -> connection ->
> {metadata,pool,match,node}
> The conn_rwlock protection of the conn-list locks on kdbus_bus is the
> only lock that doesn't follow this ordering.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
