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

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

[Jesper Juhl]

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)
 

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

[Jesper Juhl]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Zwane Mwaikambo]

On Sun, 27 Mar 2005, Jesper Juhl wrote:

> 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 ?

The concept seems fine to me, although i think you should be using named 
initialisers instead.

Thanks Jesper,

	Zwane

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

[Jesper Juhl]

On Sun, 27 Mar 2005, Zwane Mwaikambo wrote:

> On Sun, 27 Mar 2005, Jesper Juhl wrote:
> 
> > 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 ?
> 
> The concept seems fine to me, although i think you should be using named 
> initialisers instead.
> 
I wrote it the way I did to keep it similar to how the other members were 
initialized, I wouldn't mind a different approach, but doing it this way 
seemed to be what would "fit in" best, and it did the trick for me 
(silenced the annoying warnings) and seemed safe.

Thank you for taking the time to comment on this trivial issue.

-- 
Jesper

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

[Zwane Mwaikambo]

On Mon, 28 Mar 2005, Jesper Juhl wrote:

> On Sun, 27 Mar 2005, Zwane Mwaikambo wrote:
> 
> > On Sun, 27 Mar 2005, Jesper Juhl wrote:
> > 
> > > 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 ?
> > 
> > The concept seems fine to me, although i think you should be using named 
> > initialisers instead.
> > 
> I wrote it the way I did to keep it similar to how the other members were 
> initialized, I wouldn't mind a different approach, but doing it this way 
> seemed to be what would "fit in" best, and it did the trick for me 
> (silenced the annoying warnings) and seemed safe.
> 
> Thank you for taking the time to comment on this trivial issue.

Well your original patch would probably cause the least noise amongst the 
other patches changing code in that area, perhaps try pushing it.