Re: [patch] SLQB slab allocator

Nick Piggin
2009-01-23T06:14:23Z

On Wed, Jan 21, 2009 at 06:40:10PM +0100, Ingo Molnar wrote:
> -static inline void slqb_stat_inc(struct kmem_cache_list *list,
> - enum stat_item si)
> +static inline void
> +slqb_stat_inc(struct kmem_cache_list *list, enum stat_item si)
> {

Hmm, I'm not entirely fond of this style. The former scales to longer lines
with just a single style change (putting args into new lines), wheras the
latter first moves its prefixes to a newline, then moves args as the
line grows even longer.

I guess it is a matter of taste, not wrong either way... but I think most
of the mm code I'm used to looking at uses the former. Do you feel strongly?

> +static void
> +trace(struct kmem_cache *s, struct slqb_page *page, void *object, int alloc)
> {
> - if (s->flags & SLAB_TRACE) {
> - printk(KERN_INFO "TRACE %s %s 0x%p inuse=%d fp=0x%p\n",
> - s->name,
> - alloc ? "alloc" : "free",
> - object, page->inuse,
> - page->freelist);
> + if (likely(!(s->flags & SLAB_TRACE)))
> + return;

I think most of your flow control changes are improvements (others even
more than this, but this is the first one so I comment here). Thanks.

> @@ -1389,7 +1402,9 @@ static noinline void *__remote_slab_allo
> }
> if (likely(object))
> slqb_stat_inc(l, ALLOC);
> +
> spin_unlock(&n->list_lock);
> +
> return object;
> }
> #endif

Whitespace, I never really know if I'm "doing it right" or not :) And
often it is easy to tell a badly wrong one, but harder to tell what is
better between two reasonable ones. But I guess I'm the same way with
paragraphs in my writing...

> @@ -1399,12 +1414,12 @@ static noinline void *__remote_slab_allo
> *
> * Must be called with interrupts disabled.
> */
> -static __always_inline void *__slab_alloc(struct kmem_cache *s,
> - gfp_t gfpflags, int node)
> +static __always_inline void *
> +__slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node)
> {
> - void *object;
> - struct kmem_cache_cpu *c;
> struct kmem_cache_list *l;
> + struct kmem_cache_cpu *c;
> + void *object;

Same with order of local variables. You like longest lines to
shortest I know. I think I vaguely try to arrange them from the
most important or high level "actor" to the least, and then in
order of when they get discovered/used.

For example, in the above function, "object" is the raison d'etre.
kmem_cache_cpu is found first, and from that, kmem_cache_list is
found. Which slightly explains the order.

> +static __always_inline void *
> +slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node, void *addr)
> {
> - void *object;
> unsigned long flags;
> + void *object;

And here, eg. flags comes last because mostly inconsequential to
the bigger picture.

Your method is easier though, I'll grant you that :)

> static void init_kmem_cache_list(struct kmem_cache *s,
> struct kmem_cache_list *l)
> {
> - l->cache = s;
> - l->freelist.nr = 0;
> - l->freelist.head = NULL;
> - l->freelist.tail = NULL;
> - l->nr_partial = 0;
> - l->nr_slabs = 0;
> + l->cache = s;
> + l->freelist.nr = 0;
> + l->freelist.head = NULL;
> + l->freelist.tail = NULL;
> + l->nr_partial = 0;
> + l->nr_slabs = 0;
> INIT_LIST_HEAD(&l->partial);

Hmm, we seem to have gathered an extra space...

>
> #ifdef CONFIG_SMP
> - l->remote_free_check = 0;
> + l->remote_free_check = 0;
> spin_lock_init(&l->remote_free.lock);
> - l->remote_free.list.nr = 0;
> + l->remote_free.list.nr = 0;
> l->remote_free.list.head = NULL;
> l->remote_free.list.tail = NULL;
> #endif

... ah, to line up with this guy. TBH, I prefer not to religiously
line things up like this. If there is the odd long-line, just give
it the normal single space. I find it just keeps it easier to
maintain. Although you might counter that of course it is easier to
keep something clean if one relaxes their definition of "clean".

> static s8 size_index[24] __cacheline_aligned = {
> - 3, /* 8 */
> - 4, /* 16 */
> - 5, /* 24 */
> - 5, /* 32 */
> - 6, /* 40 */
> - 6, /* 48 */
> - 6, /* 56 */
> - 6, /* 64 */
> + 3, /* 8 */
> + 4, /* 16 */
> + 5, /* 24 */
> + 5, /* 32 */
> + 6, /* 40 */
> + 6, /* 48 */
> + 6, /* 56 */
> + 6, /* 64 */

However justifying numbers, like this, I'm happy to do (may as well
align the numbers in the comments too while we're here).

> @@ -2278,9 +2294,8 @@ static struct kmem_cache *get_slab(size_
>
> void *__kmalloc(size_t size, gfp_t flags)
> {
> - struct kmem_cache *s;
> + struct kmem_cache *s = get_slab(size, flags);
>
> - s = get_slab(size, flags);
> if (unlikely(ZERO_OR_NULL_PTR(s)))
> return s;

I've got yet the same problem with these... I mostly try to avoid
doing this, although there are some cases where it works well
(eg. constants, or a simple assignment of an argument to a local).

At some point, you start putting real code in there, at which point
the space after the local vars doesn't seem to serve much purpose.
get_slab I feel logically belongs close to the subsequent check,
because that's basically sanitizing its return value / extracting
the error case from it and leaving the rest of the function to work
on the common case.

> -static int sysfs_available __read_mostly = 0;
> +static int sysfs_available __read_mostly;

These, I actually like initializing to zero explicitly. I'm pretty
sure gcc no longer makes it any more expensive than leaving out.
Yes of course everybody who knows C has to know this, but.... I
just don't feel much harm in leaving it.

Lots of good stuff, lots I'm on the fence with, some I dislike ;)
I'll concentrate on picking up the obvious ones, and get the bugs
fixed. Will see where the discussion goes with the rest.

Thanks,
Nick