Re: [RFC] spinlock_t & rwlock_t break_lock member initialization(patch seeking comments included)

From: Jesper Juhl
Date: Sun Mar 27 2005 - 04:11:05 EST



I've now been running kernels (both PREEMPT, SMP, both and without both)
with the patch below applied for a few days and I see no ill effects. I'm
still interrested in comments about wether or not something like this
makes sense and is acceptable ?

--
Jesper Juhl


On Sun, 20 Mar 2005, Jesper Juhl wrote:

>
> I'm often building the tree with gcc -W to look for potential trouble
> spots, and of course I see a lot of warning messages. I'm well aware that
> most of these don't indicate actual problems and should just be ignored,
> but the less warnings there are the easier it is to zoom in on the ones
> that might actually matter, so I try to also locate those that can be
> silenced safely even when they don't indicate a real problem - just to cut
> down on the number of warnings I have to sift through.
> One class of warnings that belong in the "not really a problem but I
> believe I can silence it without doing any harm" category are these :
>
> include/linux/wait.h:82: warning: missing initializer
> include/linux/wait.h:82: warning: (near initialization for `(anonymous).break_lock')
>
> include/asm/rwsem.h:88: warning: missing initializer
> include/asm/rwsem.h:88: warning: (near initialization for `(anonymous).break_lock')
>
> These stem from the fact that when you enable CONFIG_PREEMPT spinlock_t
> and rwlock_t each gain an extra member "break_lock", but the lock
> initialization code neglects to initialize this extra member in that case.
>
> If you enable CONFIG_PREEMPT and build with gcc -W you'll see a *lot* of
> those, since spinlocks and rwlocks are used all over the place.
>
> I've come up with a patch to that silence those warnings by making sure
> the "break_lock" member will be initialized when CONFIG_PREEMPT is
> enabled.
>
> I would like to know if a patch like this is welcome or just considered
> clutter for no real gain. I would also like to know if I've overlooked
> some implications of doing this - it seems to me that this should be
> completely safe to do and without significant overhead, but I'm also well
> aware that my knowledge of this code is quite shallow, so I need comments
> (especially since lock performance is quite performance critical, so I
> don't want to screw up here).
>
> Here's the patch I came up with - comments are very welcome.
>
>
> (no Signed-off-by since this is not intended to be merged just yet)
>
> --- linux-2.6.11-mm4-orig/include/asm-i386/spinlock.h 2005-03-02 08:37:50.000000000 +0100
> +++ linux-2.6.11-mm4/include/asm-i386/spinlock.h 2005-03-20 22:40:46.000000000 +0100
> @@ -32,7 +32,13 @@
> #define SPINLOCK_MAGIC_INIT /* */
> #endif
>
> -#define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 SPINLOCK_MAGIC_INIT }
> +#ifdef CONFIG_PREEMPT
> +#define SPINLOCK_BREAK_INIT , 0
> +#else
> +#define SPINLOCK_BREAK_INIT /* */
> +#endif
> +
> +#define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 SPINLOCK_MAGIC_INIT SPINLOCK_BREAK_INIT }
>
> #define spin_lock_init(x) do { *(x) = SPIN_LOCK_UNLOCKED; } while(0)
>
> @@ -182,7 +188,13 @@
> #define RWLOCK_MAGIC_INIT /* */
> #endif
>
> -#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }
> +#ifdef CONFIG_PREEMPT
> +#define RWLOCK_BREAK_INIT , 0
> +#else
> +#define RWLOCK_BREAK_INIT /* */
> +#endif
> +
> +#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT RWLOCK_BREAK_INIT }
>
> #define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/