lkml.org 
[lkml]   [2011]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 08/13] list: add a new LRU list type
    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


    \
     
     \ /
      Last update: 2011-08-23 11:35    [W:0.073 / U:89.668 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site