Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Atul Sowani]

There are still some files containing .lock = SPIN_LOCK_UNLOCKED type
assignments which should be converted to __SPIN_LOCK_UNLOCKED().
--- Corrected spacing between __SPIN_LOCK_UNLOCKED and ( for ARM.
--- Resubmitting the patch with spacing fixes.
--- Removed ARM related changes after Russell King's NACK.

Signed-off-by: Atul Sowani <sowani@gmail.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---

diff -uprN a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
--- a/arch/mips/kernel/vpe.c	2010-10-21 02:00:22.000000000 +0530
+++ b/arch/mips/kernel/vpe.c	2010-11-07 00:51:37.000000000 +0530
@@ -148,9 +148,9 @@ struct {
 	spinlock_t tc_list_lock;
 	struct list_head tc_list;	/* Thread contexts */
 } vpecontrol = {
-	.vpe_list_lock	= SPIN_LOCK_UNLOCKED,
+	.vpe_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.vpe_list_lock),
 	.vpe_list	= LIST_HEAD_INIT(vpecontrol.vpe_list),
-	.tc_list_lock	= SPIN_LOCK_UNLOCKED,
+	.tc_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.tc_list_lock),
 	.tc_list	= LIST_HEAD_INIT(vpecontrol.tc_list)
 };
diff -uprN a/kernel/cred.c b/kernel/cred.c
--- a/kernel/cred.c	2010-10-21 02:00:22.000000000 +0530
+++ b/kernel/cred.c	2010-11-07 00:52:22.000000000 +0530
@@ -35,7 +35,7 @@ static struct kmem_cache *cred_jar;
 static struct thread_group_cred init_tgcred = {
 	.usage	= ATOMIC_INIT(2),
 	.tgid	= 0,
-	.lock	= SPIN_LOCK_UNLOCKED,
+	.lock	= __SPIN_LOCK_UNLOCKED(init_tgcred.lock),
 };
 #endif
-- 
Atul Sowani (sowani@gmail.com)
http://savannah.gnu.org/users/atul
------------------------------------------------------------------------

Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Russell King - ARM Linux]

On Mon, Nov 15, 2010 at 10:44:36AM +0100, Arnd Bergmann wrote:
> On Monday 15 November 2010 10:24:04 Russell King - ARM Linux wrote:
> > This lock is in the percpu data area.  This means that when other CPUs
> > are brought online, and therefore other percpu areas are instantiated,
> > this lock will be mis-initialized (the pointers et.al. will be pointing
> > at the original percpu instance.)
> > 
> > So NAK, this will break.
> 
> I believe right now we only use the argument of __SPIN_LOCK_UNLOCKED
> as an identifier string for lockdep, but not for any pointers, so the
> patch would actually do what was intended, but break if we ever start
> relying on the pointer.
> 
> Maybe we can mangle the argument in some way to make sure it will break
> at build time when that happens, e.g.
> 
>      .lock   = __SPIN_LOCK_UNLOCKED(ipi_data.__lock)
> 
> This would at least let us remove SPIN_LOCK_UNLOCKED.

Or do one of the things I've suggested - one of which I've just completed
the code changes for, and results in this lock going away completely.
It's not a nice idea to have percpu data written to by 'foreign' CPUs.

Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Arnd Bergmann]

On Monday 15 November 2010 10:24:04 Russell King - ARM Linux wrote:
> > diff -uprN a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > --- a/arch/arm/kernel/smp.c   2010-10-21 02:00:22.000000000 +0530
> > +++ b/arch/arm/kernel/smp.c   2010-11-07 00:47:32.000000000 +0530
> > @@ -56,7 +56,7 @@ struct ipi_data {
> >  };
> > 
> >  static DEFINE_PER_CPU(struct ipi_data, ipi_data) = {
> > -     .lock   = SPIN_LOCK_UNLOCKED,
> > +     .lock   = __SPIN_LOCK_UNLOCKED(ipi_data.lock),
> >  };
> 
> This lock is in the percpu data area.  This means that when other CPUs
> are brought online, and therefore other percpu areas are instantiated,
> this lock will be mis-initialized (the pointers et.al. will be pointing
> at the original percpu instance.)
> 
> So NAK, this will break.

I believe right now we only use the argument of __SPIN_LOCK_UNLOCKED
as an identifier string for lockdep, but not for any pointers, so the
patch would actually do what was intended, but break if we ever start
relying on the pointer.

Maybe we can mangle the argument in some way to make sure it will break
at build time when that happens, e.g.

     .lock   = __SPIN_LOCK_UNLOCKED(ipi_data.__lock)

This would at least let us remove SPIN_LOCK_UNLOCKED.

	Arnd

Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Russell King - ARM Linux]

On Sun, Nov 14, 2010 at 12:01:06AM +0530, Atul Sowani wrote:
> There are still some files containing .lock = SPIN_LOCK_UNLOCKED type
> assignments which should be converted to __SPIN_LOCK_UNLOCKED().
> Corrected spacing between __SPIN_LOCK_UNLOCKED and ( for ARM.

Thinking about the ARM bit, I'll have to NAK this patch.

> diff -uprN a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> --- a/arch/arm/kernel/smp.c	2010-10-21 02:00:22.000000000 +0530
> +++ b/arch/arm/kernel/smp.c	2010-11-07 00:47:32.000000000 +0530
> @@ -56,7 +56,7 @@ struct ipi_data {
>  };
> 
>  static DEFINE_PER_CPU(struct ipi_data, ipi_data) = {
> -	.lock	= SPIN_LOCK_UNLOCKED,
> +	.lock	= __SPIN_LOCK_UNLOCKED(ipi_data.lock),
>  };

This lock is in the percpu data area.  This means that when other CPUs
are brought online, and therefore other percpu areas are instantiated,
this lock will be mis-initialized (the pointers et.al. will be pointing
at the original percpu instance.)

So NAK, this will break.

We can either go back to a static array for this, initialize the lock
manually (though this could be problematical), or we require to go to
per-ipi type interrupts for all SMP architectures.

Re: [PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Uwe Kleine-König]

Hi Atul,

On Sun, Nov 14, 2010 at 12:01:06AM +0530, Atul Sowani wrote:
> There are still some files containing .lock = SPIN_LOCK_UNLOCKED type
> assignments which should be converted to __SPIN_LOCK_UNLOCKED().
> Corrected spacing between __SPIN_LOCK_UNLOCKED and ( for ARM.
IMHO the last sentence shouldn't appear in the commit log.  (If you
still want to let your readers know what you changed---generally a good
idea---you can do so after the triple-dash below.)

I don't know you patch series, but I'd expect that all archs don't want
the space between __SPIN_LOCK_UNLOCKED and (.

Other than that
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe
> 
> Signed-off-by: Atul Sowani <sowani@gmail.com>
> ---
> 
> diff -uprN a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> --- a/arch/arm/kernel/smp.c	2010-10-21 02:00:22.000000000 +0530
> +++ b/arch/arm/kernel/smp.c	2010-11-07 00:47:32.000000000 +0530
> @@ -56,7 +56,7 @@ struct ipi_data {
>  };
> 
>  static DEFINE_PER_CPU(struct ipi_data, ipi_data) = {
> -	.lock	= SPIN_LOCK_UNLOCKED,
> +	.lock	= __SPIN_LOCK_UNLOCKED(ipi_data.lock),
>  };
> 
>  enum ipi_msg_type {
> diff -uprN a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
> --- a/arch/mips/kernel/vpe.c	2010-10-21 02:00:22.000000000 +0530
> +++ b/arch/mips/kernel/vpe.c	2010-11-07 00:51:37.000000000 +0530
> @@ -148,9 +148,9 @@ struct {
>  	spinlock_t tc_list_lock;
>  	struct list_head tc_list;	/* Thread contexts */
>  } vpecontrol = {
> -	.vpe_list_lock	= SPIN_LOCK_UNLOCKED,
> +	.vpe_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.vpe_list_lock),
>  	.vpe_list	= LIST_HEAD_INIT(vpecontrol.vpe_list),
> -	.tc_list_lock	= SPIN_LOCK_UNLOCKED,
> +	.tc_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.tc_list_lock),
>  	.tc_list	= LIST_HEAD_INIT(vpecontrol.tc_list)
>  };
> diff -uprN a/kernel/cred.c b/kernel/cred.c
> --- a/kernel/cred.c	2010-10-21 02:00:22.000000000 +0530
> +++ b/kernel/cred.c	2010-11-07 00:52:22.000000000 +0530
> @@ -35,7 +35,7 @@ static struct kmem_cache *cred_jar;
>  static struct thread_group_cred init_tgcred = {
>  	.usage	= ATOMIC_INIT(2),
>  	.tgid	= 0,
> -	.lock	= SPIN_LOCK_UNLOCKED,
> +	.lock	= __SPIN_LOCK_UNLOCKED(init_tgcred.lock),
>  };
>  #endif
> 
> -- 
> Atul Sowani (sowani@gmail.com)
> http://savannah.gnu.org/users/atul
> ------------------------------------------------------------------------
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2.6.36 1/1 RESEND] kernel SPIN_LOCK_UNLOCKED changed to __SPIN_LOCK_UNLOCKED()

[Atul Sowani]

There are still some files containing .lock = SPIN_LOCK_UNLOCKED type
assignments which should be converted to __SPIN_LOCK_UNLOCKED().
Corrected spacing between __SPIN_LOCK_UNLOCKED and ( for ARM.

Signed-off-by: Atul Sowani <sowani@gmail.com>
---

diff -uprN a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
--- a/arch/arm/kernel/smp.c	2010-10-21 02:00:22.000000000 +0530
+++ b/arch/arm/kernel/smp.c	2010-11-07 00:47:32.000000000 +0530
@@ -56,7 +56,7 @@ struct ipi_data {
 };

 static DEFINE_PER_CPU(struct ipi_data, ipi_data) = {
-	.lock	= SPIN_LOCK_UNLOCKED,
+	.lock	= __SPIN_LOCK_UNLOCKED(ipi_data.lock),
 };

 enum ipi_msg_type {
diff -uprN a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
--- a/arch/mips/kernel/vpe.c	2010-10-21 02:00:22.000000000 +0530
+++ b/arch/mips/kernel/vpe.c	2010-11-07 00:51:37.000000000 +0530
@@ -148,9 +148,9 @@ struct {
 	spinlock_t tc_list_lock;
 	struct list_head tc_list;	/* Thread contexts */
 } vpecontrol = {
-	.vpe_list_lock	= SPIN_LOCK_UNLOCKED,
+	.vpe_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.vpe_list_lock),
 	.vpe_list	= LIST_HEAD_INIT(vpecontrol.vpe_list),
-	.tc_list_lock	= SPIN_LOCK_UNLOCKED,
+	.tc_list_lock	= __SPIN_LOCK_UNLOCKED(vpecontrol.tc_list_lock),
 	.tc_list	= LIST_HEAD_INIT(vpecontrol.tc_list)
 };
diff -uprN a/kernel/cred.c b/kernel/cred.c
--- a/kernel/cred.c	2010-10-21 02:00:22.000000000 +0530
+++ b/kernel/cred.c	2010-11-07 00:52:22.000000000 +0530
@@ -35,7 +35,7 @@ static struct kmem_cache *cred_jar;
 static struct thread_group_cred init_tgcred = {
 	.usage	= ATOMIC_INIT(2),
 	.tgid	= 0,
-	.lock	= SPIN_LOCK_UNLOCKED,
+	.lock	= __SPIN_LOCK_UNLOCKED(init_tgcred.lock),
 };
 #endif

-- 
Atul Sowani (sowani@gmail.com)
http://savannah.gnu.org/users/atul
------------------------------------------------------------------------