Date:	Fri, 23 Jan 2009 07:14:06 +0100
From:	Nick Piggin <npiggin@...e.de>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Pekka Enberg <penberg@...helsinki.fi>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Lin Ming <ming.m.lin@...el.com>,
	"Zhang, Yanmin" <yanmin_zhang@...ux.intel.com>,
	Christoph Lameter <clameter@...r.sgi.com>
Subject: Re: [patch] SLQB slab allocator

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

Powered by blists - more mailing lists