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

From: Dave Chinner
Date: Tue Aug 23 2011 - 05:32:14 EST


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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
--
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/