Re: [patch] lockdep: annotate mm/slab.c

[Andrew Morton <akpm@osdl.org>]

On Thu, 13 Jul 2006 11:50:01 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> 
> On Thu, 13 Jul 2006, Ingo Molnar wrote:
> >  
> > +#ifdef CONFIG_LOCKDEP
> > +
> > +/*
> > + * Slab sometimes uses the kmalloc slabs to store the slab headers
> > + * for other slabs "off slab".
> > + * The locking for this is tricky in that it nests within the locks
> > + * of all other slabs in a few places; to deal with this special
> > + * locking we put on-slab caches into a separate lock-class.
> > + */
> > +static struct lock_class_key on_slab_key;
> > +
> > +static inline void init_lock_keys(struct cache_sizes *s)
> > +{
> > +	int q;
> > +
> > +	for (q = 0; q < MAX_NUMNODES; q++) {
> > +		if (!s->cs_cachep->nodelists[q] || OFF_SLAB(s->cs_cachep))
> > +			continue;
> > +		lockdep_set_class(&s->cs_cachep->nodelists[q]->list_lock,
> > +				  &on_slab_key);
> > +	}
> > +}
> > +
> > +#else
> > +static inline void init_lock_keys(struct cache_sizes *s)
> > +{
> > +}
> > +#endif
> 
> Why isn't the "on_slab_key" local to just the init_lock_keys() function, 
> and the #ifdef around it all?
> 
> Ie just
> 
> 	static inline void init_lock_keys(struct cache_sizes *s)
> 	{
> 	#ifdef CONFIG_LOCKDEP
> 		static struct lock_class_key on_slab_key;
> 		int q;
> 
> 		for (q = 0; q < MAX_NUMNODES; q++) {
> 			...
> 	#endif CONFIG_LOCKDEP
> 	}
> 
> instead?
> 

It could be wholly hidded inside a macro

#define lockdep_go_away(p) {
		static struct lock_class_key foo;
		lockdep_set_class(p, &foo);
	}

But istr suggesting that a couple of weeks ago and was given a
good-sounding reason which I forget.

At least when the code laid out as Ingo proposed, we have room for a
decent comment, which is rather desirable for this sort of thing.