Hi On Thu, Apr 16, 2015 at 10:55 PM, Al Viro <v...@zeniv.linux.org.uk> wrote: > 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.
Fixed! [1] Linus just pulled akpm#3, which includes the rcu-protection for exe-file. No more direct mmap_sem access in kdbus, anymore. >> 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. Indeed. We do allow pinning parent objects when deactivating its children. I updated my doc-drafts accordingly. >> 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... I have a patch to fix this [2]. But it's more complex than the rwsem, and requires some more review. However, it reduces the handle-locking to a minimum, such that we only lock it during setup and can reduce it to a mutex. >> * 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. Sorry, this was a mistake on my side. We do call kdbus_notify_flush() directly quite often. And it nests underneath the handle, correct. I noted this down. I did have patches to actually move the kdbus_notify_flush() call to the end of kdbus_handle_ioctl() and friends. Such so we flush all collected notifications on return to user-space, which would make the locking more obvious. However, it didn't make it much simpler, imo, so it was never applied. > 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? (Updated) Data-structure locks: * bus->notify_lock * pool->lock * match->mdb_rwlock * node->lock Updated locking order: 1) handle->rwlock 2) bus->notify_flush_lock 3) domain->lock 4) names->rwlock 5) endpoint->lock 6) bus->conn_rwlock 7) policy->entries_rwlock 8) connection->lock 9) metadata->lock * node->active read-side locks arbitrarily underneath handle->rwlock. * node->active write-side nests underneath handle->rwlock, and underneath read-side of any parent-node->active. Thanks! Much appreciated! David [1] http://cgit.freedesktop.org/~dvdhrm/linux/commit/?h=kdbus&id=f396c12ecfda1717e5f76d6b4ab11e4db232e60d [2] http://cgit.freedesktop.org/~dvdhrm/linux/commit/?h=kdbus&id=61875e1abd38a965c9f7dfca28068dd0a871961c -- 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/