Re: [PATCH 16/17] fs: Convert nr_inodes to a per-cpu counter

[Nick Piggin <npiggin@kernel.dk>]

On Thu, Sep 30, 2010 at 04:10:39PM +1000, Dave Chinner wrote:
> On Wed, Sep 29, 2010 at 09:53:22PM -0700, Andrew Morton wrote:
> > On Wed, 29 Sep 2010 22:18:48 +1000 Dave Chinner <david@fromorbit.com> wrote:
> > 
> > > From: Eric Dumazet <dada1@cosmosbay.com>
> > > 
> > > The number of inodes allocated does not need to be tied to the
> > > addition or removal of an inode to/from a list. If we are not tied
> > > to a list lock, we could update the counters when inodes are
> > > initialised or destroyed, but to do that we need to convert the
> > > counters to be per-cpu (i.e. independent of a lock). This means that
> > > we have the freedom to change the list/locking implementation
> > > without needing to care about the counters.
> > > 
> > >
> > > ...
> > >
> > > +int get_nr_inodes(void)
> > > +{
> > > +	int i;
> > > +	int sum = 0;
> > > +	for_each_possible_cpu(i)
> > > +		sum += per_cpu(nr_inodes, i);
> > > +	return sum < 0 ? 0 : sum;
> > > +}
> > 
> > This reimplements percpu_counter_sum_positive(), rather poorly

Why is it poorly?


> > If one never intends to use the approximate percpu_counter_read() then
> > one could initialise the counter with a really large batch value, for a
> > very small performance gain.

I did that to start with, and I was just looking to shave off cycles
and icache size. this_cpu_inc on x86 on a local variable is really
tiny and fast. percpu_counter does a function call which is large
and clobbers memory and registers, several branches, several loads and
stores, etc.

When it is a simple dumb statistics counter but with a critical
fastpath, this_cpu_inc just seems to be so much better.


> > > +int get_nr_inodes_unused(void)
> > > +{
> > > +	return inodes_stat.nr_unused;
> > > +}
> > >
> > > ...
> > >
> > > @@ -407,6 +407,8 @@ extern struct files_stat_struct files_stat;
> > >  extern int get_max_files(void);
> > >  extern int sysctl_nr_open;
> > >  extern struct inodes_stat_t inodes_stat;
> > > +extern int get_nr_inodes(void);
> > > +extern int get_nr_inodes_unused(void);
> > 
> > These are pretty cruddy names.  Unfotunately we don't really have a vfs
> > or "inode" subsystem name to prefix them with.

Any ideas? inodes_stat_nr_unused()?