Re: [PATCH 03/23] vfs: Generalize the file_list

[Nick Piggin]

On Tue, Jun 09, 2009 at 11:38:59AM -0700, Eric W. Biederman wrote:
> Nick Piggin <npiggin@xxxxxxx> writes:
> 
> > On Fri, Jun 05, 2009 at 12:33:59PM -0700, Eric W. Biederman wrote:
> >> Nick Piggin <npiggin@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/