Define __raw_read_lock etc for uniprocessor builds

Define __raw_read_lock etc for uniprocessor builds

[Joe Korty]

Make NOPed versions of __raw_read_lock and family available
under uniprocessor kernels.

Discovered when compiling a uniprocessor kernel with the
fusyn patch applied.

The standard kernel does not use __raw_read_lock etc
outside of spinlock.c, which may account for this bug
being undiscovered until now.


 2.6.16-rc1-git4-jak/include/linux/spinlock_up.h |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff -puNa include/linux/spinlock_up.h~define.__raw_read_lock.and.family.for.uniproc-nodebug.combo include/linux/spinlock_up.h
--- 2.6.16-rc1-git4/include/linux/spinlock_up.h~define.__raw_read_lock.and.family.for.uniproc-nodebug.combo	2006-01-24 12:57:15.000000000 -0500
+++ 2.6.16-rc1-git4-jak/include/linux/spinlock_up.h	2006-01-24 12:58:51.000000000 -0500
@@ -47,16 +47,6 @@ static inline void __raw_spin_unlock(raw
 	lock->slock = 1;
 }
 
-/*
- * Read-write spinlocks. No debug version.
- */
-#define __raw_read_lock(lock)		do { (void)(lock); } while (0)
-#define __raw_write_lock(lock)		do { (void)(lock); } while (0)
-#define __raw_read_trylock(lock)	({ (void)(lock); 1; })
-#define __raw_write_trylock(lock)	({ (void)(lock); 1; })
-#define __raw_read_unlock(lock)		do { (void)(lock); } while (0)
-#define __raw_write_unlock(lock)	do { (void)(lock); } while (0)
-
 #else /* DEBUG_SPINLOCK */
 #define __raw_spin_is_locked(lock)	((void)(lock), 0)
 /* for sched.c and kernel_lock.c: */
@@ -71,4 +61,14 @@ static inline void __raw_spin_unlock(raw
 #define __raw_spin_unlock_wait(lock) \
 		do { cpu_relax(); } while (__raw_spin_is_locked(lock))
 
+/*
+ * Read-write spinlocks. Only non-debug versions available.
+ */
+#define __raw_read_lock(lock)		do { (void)(lock); } while (0)
+#define __raw_write_lock(lock)		do { (void)(lock); } while (0)
+#define __raw_read_trylock(lock)	({ (void)(lock); 1; })
+#define __raw_write_trylock(lock)	({ (void)(lock); 1; })
+#define __raw_read_unlock(lock)		do { (void)(lock); } while (0)
+#define __raw_write_unlock(lock)	do { (void)(lock); } while (0)
+
 #endif /* __LINUX_SPINLOCK_UP_H */

_

Re: Define __raw_read_lock etc for uniprocessor builds

[Christoph Hellwig]

On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> 
> Make NOPed versions of __raw_read_lock and family available
> under uniprocessor kernels.
> 
> Discovered when compiling a uniprocessor kernel with the
> fusyn patch applied.
> 
> The standard kernel does not use __raw_read_lock etc
> outside of spinlock.c, which may account for this bug
> being undiscovered until now.

No one should call these directly.   Please fix your odd patch instead.

Re: Define __raw_read_lock etc for uniprocessor builds

[Joe Korty]

On Tue, Jan 24, 2006 at 06:17:12PM +0000, Christoph Hellwig wrote:
> On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> > 
> > Make NOPed versions of __raw_read_lock and family available
> > under uniprocessor kernels.
> > 
> > Discovered when compiling a uniprocessor kernel with the
> > fusyn patch applied.
> > 
> > The standard kernel does not use __raw_read_lock etc
> > outside of spinlock.c, which may account for this bug
> > being undiscovered until now.
> 
> No one should call these directly.   Please fix your odd patch instead.

Actually the patch calls the _raw version which is #defined to the __raw
version.  So it is doing the correct thing.

Joe

Re: Define __raw_read_lock etc for uniprocessor builds

[Arjan van de Ven]

On Tue, 2006-01-24 at 13:29 -0500, Joe Korty wrote:
> On Tue, Jan 24, 2006 at 06:17:12PM +0000, Christoph Hellwig wrote:
> > On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> > > 
> > > Make NOPed versions of __raw_read_lock and family available
> > > under uniprocessor kernels.
> > > 
> > > Discovered when compiling a uniprocessor kernel with the
> > > fusyn patch applied.
> > > 
> > > The standard kernel does not use __raw_read_lock etc
> > > outside of spinlock.c, which may account for this bug
> > > being undiscovered until now.
> > 
> > No one should call these directly.   Please fix your odd patch instead.
> 
> Actually the patch calls the _raw version which is #defined to the __raw
> version.  So it is doing the correct thing.

no it's not, it has no business calling the _raw version either.

Re: Define __raw_read_lock etc for uniprocessor builds

[Joe Korty]

On Tue, Jan 24, 2006 at 07:32:02PM +0100, Arjan van de Ven wrote:
> On Tue, 2006-01-24 at 13:29 -0500, Joe Korty wrote:
> > On Tue, Jan 24, 2006 at 06:17:12PM +0000, Christoph Hellwig wrote:
> > > On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> > > > 
> > > > Make NOPed versions of __raw_read_lock and family available
> > > > under uniprocessor kernels.
> > > > 
> > > > Discovered when compiling a uniprocessor kernel with the
> > > > fusyn patch applied.
> > > > 
> > > > The standard kernel does not use __raw_read_lock etc
> > > > outside of spinlock.c, which may account for this bug
> > > > being undiscovered until now.
> > > 
> > > No one should call these directly.   Please fix your odd patch instead.
> > 
> > Actually the patch calls the _raw version which is #defined to the __raw
> > version.  So it is doing the correct thing.
> 
> no it's not, it has no business calling the _raw version either.

Nope.

1) The _raw_spin_lock family is used everywhere in the kernel.  Why the
arbitrary special rule for _raw_read_lock??????  It makes no sense.

2) The _raw versions are intended to be used in places where it is
known that preemption is already disabled, so that the overhead of
re-disabling/enabling it can be avoided.

Joe
--
"All the revision in the world will not save a bad first draft, for the
architecture of the thing comes, or fails to come, in the first conception,
and revision only affects the detail and ornament. -- T.E. Lawrence

Re: Define __raw_read_lock etc for uniprocessor builds

[Arjan van de Ven]

On Tue, 2006-01-24 at 13:38 -0500, Joe Korty wrote:
> On Tue, Jan 24, 2006 at 07:32:02PM +0100, Arjan van de Ven wrote:
> > On Tue, 2006-01-24 at 13:29 -0500, Joe Korty wrote:
> > > On Tue, Jan 24, 2006 at 06:17:12PM +0000, Christoph Hellwig wrote:
> > > > On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> > > > > 
> > > > > Make NOPed versions of __raw_read_lock and family available
> > > > > under uniprocessor kernels.
> > > > > 
> > > > > Discovered when compiling a uniprocessor kernel with the
> > > > > fusyn patch applied.
> > > > > 
> > > > > The standard kernel does not use __raw_read_lock etc
> > > > > outside of spinlock.c, which may account for this bug
> > > > > being undiscovered until now.
> > > > 
> > > > No one should call these directly.   Please fix your odd patch instead.
> > > 
> > > Actually the patch calls the _raw version which is #defined to the __raw
> > > version.  So it is doing the correct thing.
> > 
> > no it's not, it has no business calling the _raw version either.
> 
> Nope.
> 
> 1) The _raw_spin_lock family is used everywhere in the kernel.  

no it's not. It's used in a few very special architecture places, and in
the spinlock.c code, and in one place of the scheduler, which is
arguably special.

I don't know what kernel you're looking at.. but it's not a kernel.org
one.


> 2) The _raw versions are intended to be used in places where it is
> known that preemption is already disabled, so that the overhead of
> re-disabling/enabling it can be avoided.

that's not true either. If it was, then the name would have been
different.

Re: Define __raw_read_lock etc for uniprocessor builds

[Joe Korty]

On Tue, Jan 24, 2006 at 07:42:36PM +0100, Arjan van de Ven wrote:
> On Tue, 2006-01-24 at 13:38 -0500, Joe Korty wrote:
> > On Tue, Jan 24, 2006 at 07:32:02PM +0100, Arjan van de Ven wrote:
> > > On Tue, 2006-01-24 at 13:29 -0500, Joe Korty wrote:
> > > > On Tue, Jan 24, 2006 at 06:17:12PM +0000, Christoph Hellwig wrote:
> > > > > On Tue, Jan 24, 2006 at 01:09:54PM -0500, Joe Korty wrote:
> > > > > > 
> > > > > > Make NOPed versions of __raw_read_lock and family available
> > > > > > under uniprocessor kernels.
> > > > > > 
> > > > > > Discovered when compiling a uniprocessor kernel with the
> > > > > > fusyn patch applied.
> > > > > > 
> > > > > > The standard kernel does not use __raw_read_lock etc
> > > > > > outside of spinlock.c, which may account for this bug
> > > > > > being undiscovered until now.
> > > > > 
> > > > > No one should call these directly.   Please fix your odd patch instead.
> > > > 
> > > > Actually the patch calls the _raw version which is #defined to the __raw
> > > > version.  So it is doing the correct thing.
> > > 
> > > no it's not, it has no business calling the _raw version either.
> > 
> > Nope.
> > 
> > 1) The _raw_spin_lock family is used everywhere in the kernel.  
> 
> no it's not. It's used in a few very special architecture places, and in
> the spinlock.c code, and in one place of the scheduler, which is
> arguably special.
> 
> I don't know what kernel you're looking at.. but it's not a kernel.org
> one.
> 
> 
> > 2) The _raw versions are intended to be used in places where it is
> > known that preemption is already disabled, so that the overhead of
> > re-disabling/enabling it can be avoided.
> 
> that's not true either. If it was, then the name would have been
> different.

I'll leave it to Ingo to decide.  After all, the NOPed versions are in the
tree already and have been for some time.  They just are under the wrong
#ifdef, so it seems like it was his intent to provide it, but failed to
do so in a way that actually enabled them.

(and '_raw'  is a perfect prefix for the core spinlock services).

Joe

Re: Define __raw_read_lock etc for uniprocessor builds

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> > 2) The _raw versions are intended to be used in places where it is
> > known that preemption is already disabled, so that the overhead of
> > re-disabling/enabling it can be avoided.
> 
> that's not true either. If it was, then the name would have been 
> different.

indeed. The __raw versions are _not_ to be used by anything else but the 
generic spinlock code. (There's one other stray use by PARISC, but that 
is justified.)

[ the -rt tree makes use of 'raw_' spinlocks, but they are totally
  different things. ]

	Ingo

Re: Define __raw_read_lock etc for uniprocessor builds

[Ingo Molnar]

* Joe Korty <joe.korty@ccur.com> wrote:

> Make NOPed versions of __raw_read_lock and family available under 
> uniprocessor kernels.

NACK. The __raw ops are only used by the spinlock implementation, and 
only on SMP.

	Ingo