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

[Eric W. Biederman]

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.

>> > 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.

Eric
--
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/