lkml.org 
[lkml]   [2006]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC 1/4] slab freeing consolidation
    HI,

    On Mon, 19 Jun 2006, Christoph Lameter wrote:
    > Add a new function drop_freelist that removes slabs with objects
    > that are already free and use that in various places.

    I think you mean drain_freelist(). Anyway, looks good. Some
    minor comments below.

    > -static int __node_shrink(struct kmem_cache *cachep, int node)
    > +/*
    > + * Remove slabs from the list of free slabs.
    > + * Specify the number of slabs to drain in tofree.
    > + *
    > + * Returns the actual number of slabs released.
    > + */
    > +static int long drain_freelist(struct kmem_cache *cachep,
    > + struct kmem_list3 *l3, int tofree)

    I have been trying to slowly kill the 'p' prefix so I'd appreciate if you
    could just call the parameter 'cache'. Also, l3 could be 'lists'.

    > + nr_freed = 0;
    > + while (nr_freed < tofree && !list_empty(&l3->slabs_free)) {
    >
    > - p = l3->slabs_free.prev;
    > - if (p == &l3->slabs_free)
    > - break;
    > + spin_lock_irq(&l3->list_lock);
    > + p = l3->slabs_free.next;
    > + if (p == &(l3->slabs_free)) {

    Please drop the redundant parenthesis.

    > + spin_unlock_irq(&l3->list_lock);
    > + return nr_freed;
    > + }

    Goto to the bottom would be nicer than return here, maybe.

    >
    > - slabp = list_entry(l3->slabs_free.prev, struct slab, list);
    > + slabp = list_entry(p, struct slab, list);
    > #if DEBUG
    > BUG_ON(slabp->inuse);
    > #endif
    > list_del(&slabp->list);
    > -
    > + /*
    > + * Safe to drop the lock. The slab is no longer linked
    > + * to the cache.
    > + */
    > l3->free_objects -= cachep->num;
    > spin_unlock_irq(&l3->list_lock);
    > slab_destroy(cachep, slabp);
    > - spin_lock_irq(&l3->list_lock);
    > - }
    > - ret = !list_empty(&l3->slabs_full) || !list_empty(&l3->slabs_partial);
    > - return ret;
    > + nr_freed ++;

    Redundant whitespace.

    > + };

    Redundant semicolon.

    > + else {
    > + int x;

    nr_freed, would be better.

    >
    > - slabp = list_entry(p, struct slab, list);
    > - BUG_ON(slabp->inuse);
    > - list_del(&slabp->list);
    > - STATS_INC_REAPED(searchp);
    > -
    > - /*
    > - * Safe to drop the lock. The slab is no longer linked
    > - * to the cache. searchp cannot disappear, we hold
    > - * cache_chain_lock
    > - */
    > - l3->free_objects -= searchp->num;
    > - spin_unlock_irq(&l3->list_lock);
    > - slab_destroy(searchp, slabp);
    > - } while (--tofree > 0);
    > + x = drain_freelist(searchp, l3, (l3->free_limit +
    > + 5 * searchp->num - 1) / (5 * searchp->num));
    > + STATS_ADD_REAPED(searchp, x);

    Maybe extract a local variable 'to_free' for readability.

    Pekka
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-06-22 21:20    [W:4.213 / U:0.936 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site