Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755066AbZFJGFX (ORCPT ); Wed, 10 Jun 2009 02:05:23 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751493AbZFJGFM (ORCPT ); Wed, 10 Jun 2009 02:05:12 -0400 Received: from cantor2.suse.de ([195.135.220.15]:36421 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbZFJGFK (ORCPT ); Wed, 10 Jun 2009 02:05:10 -0400 Date: Wed, 10 Jun 2009 08:05:11 +0200 From: Nick Piggin To: "Eric W. Biederman" Cc: Al Viro , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Hugh Dickins , Tejun Heo , Alexey Dobriyan , Linus Torvalds , Alan Cox , Greg Kroah-Hartman , Andrew Morton , Christoph Hellwig , "Eric W. Biederman" Subject: Re: [PATCH 03/23] vfs: Generalize the file_list Message-ID: <20090610060511.GA31155@wotan.suse.de> References: <1243893048-17031-3-git-send-email-ebiederm@xmission.com> <20090602070642.GD31556@wotan.suse.de> <20090609103832.GI14820@wotan.suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.9i Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3634 Lines: 88 On Tue, Jun 09, 2009 at 11:38:59AM -0700, Eric W. Biederman wrote: > Nick Piggin writes: > > > On Fri, Jun 05, 2009 at 12:33:59PM -0700, Eric W. Biederman wrote: > >> Nick Piggin writes: > >> > >> >> +static inline void file_list_unlock(struct file_list *files) > >> >> +{ > >> >> + spin_unlock(&files->lock); > >> >> +} > >> > > >> > I don't really like this. It's just a list head. Get rid of > >> > all these wrappers and crap I'd say. In fact, starting with my > >> > patch to unexport files_lock and remove these wrappers would > >> > be reasonable, wouldn't it? > >> > >> I don't really mind killing the wrappers. > >> > >> I do mind your patch because it makes the list going through > >> the tty's something very different. In my view of the world > >> that is the only use case is what I'm working to move up more > >> into the vfs layer. So orphaning it seems wrong. > > > > My patch doesn't orphan it, it just makes the locking more > > explicit and that's all so it should be easier to work with. > > I just mean start with my patch and you could change things > > as needed. > > As I recall you weren't using the files_lock for the tty layer. I > seem to recall you were still walking through the same list head on > struct file. > > Regardless it sure felt like pushing the tty usage out into > some weird special case. My goal is to make it reasonable for > more character drivers to use the list so it isn't an especially > comfortable starting place for me. I don't see the problem. It made files_lock for filesystems and uses another lock for tty. Tty is a special case (or different case) compared with filesystem, and how did it make it unreasonable for character drivers to use the list? Mandating the locking and list to be in the inode for everyone is just bloating things up. > >> > Increasing the size of the struct inode by 24 bytes hurts. > >> > Even when you decrapify it and can reuse i_lock or something, > >> > then it is still 16 bytes on 64-bit. > >> > >> We can get it even smaller if we make it an hlist. A hlist_head is > >> only a single pointer. This size growth appears to be one of the > >> biggest weakness of the code. > > > > 8 bytes would be a lot better than 24. > > Definitely. > > >> > I haven't looked through all the patches... but this is to > >> > speed up a slowpath operation, isn't it? Or does revoke > >> > need to be especially performant? > >> > >> This was more about simplicity rather than performance. The > >> performance gain is using a per inode lock instead of a global lock. > >> Which keeps cache lines from bouncing. > > > > Yes but we already have such a global lock which has been > > OK until now. Granted that some users are running into these > > locks, but fine graining them can be considered independently > > I think. So using per-sb lists of files and not bloating > > struct inode any more could be a less controversial step > > for you. > > I will take a look. Certainly doing the work in a couple > of patches seems reasonable. If I can move all of the list > maintenance out of the tty layer. That looks to be the ideal > case. I will wait to see. It will be nice if you have any obvious standalone fixes or improvements then to post them first or in front of your patchset: I'd like to make some progress here too to help my locking patchset. -- 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/