Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754136AbbDPU4Z (ORCPT ); Thu, 16 Apr 2015 16:56:25 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:38385 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753954AbbDPU4V (ORCPT ); Thu, 16 Apr 2015 16:56:21 -0400 Date: Thu, 16 Apr 2015 21:55:57 +0100 From: Al Viro To: David Herrmann Cc: Greg Kroah-Hartman , Jiri Kosina , Borislav Petkov , Andy Lutomirski , Linus Torvalds , Andrew Morton , Arnd Bergmann , "Eric W. Biederman" , One Thousand Gnomes , Tom Gundersen , "linux-kernel@vger.kernel.org" , Daniel Mack , Djalal Harouni Subject: Re: [GIT PULL] kdbus for 4.1-rc1 Message-ID: <20150416205557.GX889@ZenIV.linux.org.uk> References: <20150414175019.GA2874@kroah.com> <20150414192357.GA6107@kroah.com> <20150414193533.GF889@ZenIV.linux.org.uk> <20150414194348.GA7540@kroah.com> <552EA700.7000200@gmail.com> <20150415232218.7df214ba@lxorguk.ukuu.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3977 Lines: 90 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/