Re: [PATCH 08/13] list: add a new LRU list type

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 23, 2011 at 05:20:56AM -0400, Christoph Hellwig wrote:
> On Tue, Aug 23, 2011 at 06:56:21PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Several subsystems use the same construct for LRU lists - a list
> > head, a spin lock and and item count. They also use exactly the same
> > code for adding and removing items from the LRU. Create a generic
> > type for these LRU lists.
> > 
> > This is the beginning of generic, node aware LRUs for shrinkers to
> > work with.
> 
> Why list_lru vs the more natural sounding lru_list?

because the mmzone.h claimed that namespace:

enum lru_list {
        LRU_INACTIVE_ANON = LRU_BASE,
        LRU_ACTIVE_ANON = LRU_BASE + LRU_ACTIVE,
        LRU_INACTIVE_FILE = LRU_BASE + LRU_FILE,
        LRU_ACTIVE_FILE = LRU_BASE + LRU_FILE + LRU_ACTIVE,
        LRU_UNEVICTABLE,
        NR_LRU_LISTS
};

and it is widely spewed through the mm code. I didn't really feel
like having to clean that mess up first....

> > diff --git a/lib/Makefile b/lib/Makefile
> > index d5d175c..a08212f 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -12,7 +12,8 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
> >  	 idr.o int_sqrt.o extable.o prio_tree.o \
> >  	 sha1.o md5.o irq_regs.o reciprocal_div.o argv_split.o \
> >  	 proportions.o prio_heap.o ratelimit.o show_mem.o \
> > -	 is_single_threaded.o plist.o decompress.o find_next_bit.o
> > +	 is_single_threaded.o plist.o decompress.o find_next_bit.o \
> > +	 list_lru.o
> 
> Di we finally fix the issues with lib-y objects beeing discarded despite
> modules relying on the exports?

Don't care. The list_lru code is used by the VFs, so it will always
be built in....

> > +int
> > +list_lru_add(
> > +	struct list_lru	*lru,
> > +	struct list_head *item)
> > +{
> 
> What about some kerneldoc comments for the helpers?

Yup, to be done.

> 
> > +		ret = isolate(item, &lru->lock, cb_arg);
> > +		switch (ret) {
> > +		case 0:	/* item removed from list */
> > +			lru->nr_items--;
> > +			removed++;
> > +			break;
> > +		case 1: /* item referenced, give another pass */
> > +			list_move_tail(item, &lru->list);
> > +			break;
> > +		case 2: /* item cannot be locked, skip */
> > +			break;
> > +		case 3: /* item not freeable, lock dropped */
> > +			goto restart;
> 
> I think the isolate callback returns shoud have symbolic names, i.e.
> and enum lru_isolate or similar.

Will do.

> 
> > +int
> > +list_lru_init(
> > +	struct list_lru	*lru)
> > +{
> > +	spin_lock_init(&lru->lock);
> > +	INIT_LIST_HEAD(&lru->list);
> > +	lru->nr_items = 0;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(list_lru_init);
> 
> This one doesn't need a return value.

No, not yet. I'll kill it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com