Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754719AbbDOMha (ORCPT ); Wed, 15 Apr 2015 08:37:30 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:34956 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754392AbbDOMgy (ORCPT ); Wed, 15 Apr 2015 08:36:54 -0400 Date: Wed, 15 Apr 2015 13:36:33 +0100 From: Al Viro To: Greg Kroah-Hartman Cc: 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 , David Herrmann , Djalal Harouni Subject: Re: [GIT PULL] kdbus for 4.1-rc1 Message-ID: <20150415123632.GJ889@ZenIV.linux.org.uk> References: <20150414192357.GA6107@kroah.com> <20150414192429.GC26075@pd.tnic> <20150414193229.GB6107@kroah.com> <20150414194004.GG889@ZenIV.linux.org.uk> <20150414194804.GB7540@kroah.com> <20150414195336.GG14069@pd.tnic> <20150415084440.GF16381@kroah.com> <20150415090948.GA17347@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150415090948.GA17347@kroah.com> 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: 2929 Lines: 57 On Wed, Apr 15, 2015 at 11:09:48AM +0200, Greg Kroah-Hartman wrote: > I've asked for it, but finding people to review code is hard, as you > know. It's only 13k lines long, smaller than a serial port driver (my > unit of code review), so it's not all that big. > > It's smaller than the USB3 host controller driver as well, and very few > people ever reviewed that beast :) > > > For something that's potentially such a core mechanism as a completely > > new, massively-adopted IPC, this does send a warning singal. > > If you know of a way to force others to review code, please let me know. Have it in a less nasty state, perhaps? Random question: al@duke:~/linux/trees/vfs$ git grep -n -w kdbus_node_idr_lock ipc/kdbus/node.c:237:static DECLARE_RWSEM(kdbus_node_idr_lock); ipc/kdbus/node.c:340: down_write(&kdbus_node_idr_lock); ipc/kdbus/node.c:344: up_write(&kdbus_node_idr_lock); ipc/kdbus/node.c:444: down_write(&kdbus_node_idr_lock); ipc/kdbus/node.c:452: up_write(&kdbus_node_idr_lock); Do you see anything wrong with that? Or with things like that: mutex_lock(&pos->lock); v_pre = atomic_read(&pos->active); if (v_pre >= 0) atomic_add_return(KDBUS_NODE_BIAS, &pos->active); else if (v_pre == KDBUS_NODE_NEW) atomic_set(&pos->active, KDBUS_NODE_RELEASE_DIRECT); mutex_unlock(&pos->lock); What are the locking rules for ->active/->waitq/->lock? Are those the outermost thing in the hierarchy? Or is that dependent on the node location? It sure as hell is outside of (at least) ->mmap_sem (by way of kdbus_conn_connect() establishing that ->active/->waitq is outside of ->conn_rwlock, which due to kdbus_bus_broadcast() nests outside of anything taken by kdbus_meta_proc_collect(), which includes ->mmap_sem) and that alone brings in a lot... Document your goddamn locking, would you? It *IS* new code, and you, as you say, had very few people working on it, so you don't have the excuses for the mess existing in older parts of the tree. Locking complexity in there is easily as bad as that of VFS sans the RCU fun; sure, I can spend a week and (hopefully) document it for you, but I would really prefer if you guys had done that. And I *do* appreciate the comments in node.c, but they are nowhere near enough. Tracking the call chains in there and trying to derive the locking ordering from those is quite a bit of work; _verifying_ that it matches the claimed one would be expected from reviewers, but as it is you are asking to spend a lot of efforts to close the gaps in your documentation. Sheesh... -- 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/