2007-01-31 13:33:55

by Robert P. J. Day

[permalink] [raw]
Subject: [PATCH] Centralize the selection for debugging semaphores.


Centralize the kernel config option for debugging semaphores and
modify the macro for frv to use that config option instead.

Signed-off-by: Robert P. J. Day <[email protected]>

---

at the moment, two architectures have support for debugging
semaphores: alpha (with a local Kconfig.debug option), and frv (with
a hard-coded "SEMAPHORE_DEBUG" macro).

it would seem to make more sense to simply have a universal kernel
config option for debugging semaphores in lib/Kconfig.debug, and let
any architecture work off of that.

obviously, only two architectures would take advantage of it right
now, but the "help" screen for the new config option mentions that,
and it's trivial to add new architectures.


arch/alpha/Kconfig.debug | 8 --------
arch/frv/kernel/semaphore.c | 2 +-
include/asm-frv/semaphore.h | 14 ++++++--------
lib/Kconfig.debug | 11 +++++++++++
4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/alpha/Kconfig.debug b/arch/alpha/Kconfig.debug
index 36d0106..f45f28c 100644
--- a/arch/alpha/Kconfig.debug
+++ b/arch/alpha/Kconfig.debug
@@ -16,14 +16,6 @@ config DEBUG_RWLOCK
too many attempts. If you suspect a rwlock problem or a kernel
hacker asks for this option then say Y. Otherwise say N.

-config DEBUG_SEMAPHORE
- bool "Semaphore debugging"
- depends on DEBUG_KERNEL
- help
- If you say Y here then semaphore processing will issue lots of
- verbose debugging messages. If you suspect a semaphore problem or a
- kernel hacker asks for this option then say Y. Otherwise say N.
-
config ALPHA_LEGACY_START_ADDRESS
bool "Legacy kernel start address"
depends on ALPHA_GENERIC
diff --git a/arch/frv/kernel/semaphore.c b/arch/frv/kernel/semaphore.c
index f278cdf..8e182ce 100644
--- a/arch/frv/kernel/semaphore.c
+++ b/arch/frv/kernel/semaphore.c
@@ -19,7 +19,7 @@ struct sem_waiter {
struct task_struct *task;
};

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
void semtrace(struct semaphore *sem, const char *str)
{
if (sem->debug)
diff --git a/include/asm-frv/semaphore.h b/include/asm-frv/semaphore.h
index 907c5c3..0958652 100644
--- a/include/asm-frv/semaphore.h
+++ b/include/asm-frv/semaphore.h
@@ -20,8 +20,6 @@
#include <linux/spinlock.h>
#include <linux/rwsem.h>

-#define SEMAPHORE_DEBUG 0
-
/*
* the semaphore definition
* - if counter is >0 then there are tokens available on the semaphore for down to collect
@@ -32,12 +30,12 @@ struct semaphore {
unsigned counter;
spinlock_t wait_lock;
struct list_head wait_list;
-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
unsigned __magic;
#endif
};

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
# define __SEM_DEBUG_INIT(name) , (long)&(name).__magic
#else
# define __SEM_DEBUG_INIT(name)
@@ -76,7 +74,7 @@ static inline void down(struct semaphore *sem)
{
unsigned long flags;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
CHECK_MAGIC(sem->__magic);
#endif

@@ -95,7 +93,7 @@ static inline int down_interruptible(struct semaphore *sem)
unsigned long flags;
int ret = 0;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
CHECK_MAGIC(sem->__magic);
#endif

@@ -119,7 +117,7 @@ static inline int down_trylock(struct semaphore *sem)
unsigned long flags;
int success = 0;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
CHECK_MAGIC(sem->__magic);
#endif

@@ -136,7 +134,7 @@ static inline void up(struct semaphore *sem)
{
unsigned long flags;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
CHECK_MAGIC(sem->__magic);
#endif

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5c26818..17c4524 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -181,6 +181,17 @@ config DEBUG_MUTEXES
This feature allows mutex semantics violations to be detected and
reported.

+config DEBUG_SEMAPHORE
+ bool "Semaphore debugging"
+ depends on DEBUG_KERNEL
+ default n
+ help
+ If you say Y here then semaphore processing will issue lots of
+ verbose debugging messages. If you suspect a semaphore problem or a
+ kernel hacker asks for this option then say Y. Otherwise say N.
+
+ At the moment, this is implemented only by alpha and frv.
+
config DEBUG_RWSEMS
bool "RW-sem debugging: basic checks"
depends on DEBUG_KERNEL

--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================


2007-01-31 13:44:19

by Michal Piotrowski

[permalink] [raw]
Subject: Re: [PATCH] Centralize the selection for debugging semaphores.

On 31/01/07, Robert P. J. Day <[email protected]> wrote:
>
> Centralize the kernel config option for debugging semaphores and
> modify the macro for frv to use that config option instead.
>
> Signed-off-by: Robert P. J. Day <[email protected]>
[..]
> +config DEBUG_SEMAPHORE
> + bool "Semaphore debugging"
> + depends on DEBUG_KERNEL
> + default n
> + help
> + If you say Y here then semaphore processing will issue lots of
> + verbose debugging messages. If you suspect a semaphore problem or a
> + kernel hacker asks for this option then say Y. Otherwise say N.
> +
> + At the moment, this is implemented only by alpha and frv.

IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug

Regards,
Michal

--
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/)

2007-01-31 13:55:51

by Robert P. J. Day

[permalink] [raw]
Subject: Re: [PATCH] Centralize the selection for debugging semaphores.

On Wed, 31 Jan 2007, Michal Piotrowski wrote:

> On 31/01/07, Robert P. J. Day <[email protected]> wrote:
> >
> > Centralize the kernel config option for debugging semaphores and
> > modify the macro for frv to use that config option instead.
> >
> > Signed-off-by: Robert P. J. Day <[email protected]>
> [..]
> > +config DEBUG_SEMAPHORE
> > + bool "Semaphore debugging"
> > + depends on DEBUG_KERNEL
> > + default n
> > + help
> > + If you say Y here then semaphore processing will issue lots of
> > + verbose debugging messages. If you suspect a semaphore problem or
> > a
> > + kernel hacker asks for this option then say Y. Otherwise say N.
> > +
> > + At the moment, this is implemented only by alpha and frv.
>
> IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug

any particular reason? and note that it's currently *not* in
arch/frv/Kconfig.debug -- it's hard-coded with a macro name that's not
the same as the config option, and i don't really see the advantage of
having each architecture implement precisely the same feature in
different and incompatible ways. but i'm willing to be convinced
otherwise.

rday

--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================

2007-01-31 14:00:04

by Michal Piotrowski

[permalink] [raw]
Subject: Re: [PATCH] Centralize the selection for debugging semaphores.

On 31/01/07, Robert P. J. Day <[email protected]> wrote:
> On Wed, 31 Jan 2007, Michal Piotrowski wrote:
>
> > On 31/01/07, Robert P. J. Day <[email protected]> wrote:
> > >
> > > Centralize the kernel config option for debugging semaphores and
> > > modify the macro for frv to use that config option instead.
> > >
> > > Signed-off-by: Robert P. J. Day <[email protected]>
> > [..]
> > > +config DEBUG_SEMAPHORE
> > > + bool "Semaphore debugging"
> > > + depends on DEBUG_KERNEL
> > > + default n
> > > + help
> > > + If you say Y here then semaphore processing will issue lots of
> > > + verbose debugging messages. If you suspect a semaphore problem or
> > > a
> > > + kernel hacker asks for this option then say Y. Otherwise say N.
> > > +
> > > + At the moment, this is implemented only by alpha and frv.
> >
> > IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug
>
> any particular reason?

Only this
"At the moment, this is implemented only by alpha and frv."

It depends on architecture.

> and note that it's currently *not* in
> arch/frv/Kconfig.debug -- it's hard-coded with a macro name that's not
> the same as the config option, and i don't really see the advantage of
> having each architecture implement precisely the same feature in
> different and incompatible ways. but i'm willing to be convinced
> otherwise.
>
> rday

Regards,
Michal

--
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/)

2007-01-31 14:21:52

by Robert P. J. Day

[permalink] [raw]
Subject: Re: [PATCH] Centralize the selection for debugging semaphores.

On Wed, 31 Jan 2007, Michal Piotrowski wrote:

> On 31/01/07, Robert P. J. Day <[email protected]> wrote:
> > On Wed, 31 Jan 2007, Michal Piotrowski wrote:
> >
> > > On 31/01/07, Robert P. J. Day <[email protected]> wrote:
> > > >
> > > > Centralize the kernel config option for debugging semaphores and
> > > > modify the macro for frv to use that config option instead.
> > > >
> > > > Signed-off-by: Robert P. J. Day <[email protected]>
> > > [..]
> > > > +config DEBUG_SEMAPHORE
> > > > + bool "Semaphore debugging"
> > > > + depends on DEBUG_KERNEL
> > > > + default n
> > > > + help
> > > > + If you say Y here then semaphore processing will issue lots of
> > > > + verbose debugging messages. If you suspect a semaphore
> > problem or
> > > > a
> > > > + kernel hacker asks for this option then say Y. Otherwise say
> > N.
> > > > +
> > > > + At the moment, this is implemented only by alpha and frv.
> > >
> > > IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug
> >
> > any particular reason?
>
> Only this
> "At the moment, this is implemented only by alpha and frv."
>
> It depends on architecture.

yes, i realize that, but surely there's a cleaner way to implement
something that's supported by only *some* of the architectures rather
than duplicating the code for every one of those architectures.

one possibility is to have the global config option look like this:

config DEBUG_SEMAPHORE
bool "Semaphore debugging"
depends on DEBUG_KERNEL && HAVE_SEMAPHORE_DEBUGGING
^^^^^^^^^^^^^^^^^^^^^^^^
default n
...

at that point, any architecture that implements semaphore debugging
and wants to advertise that simply needs to add this at the top of its
Kconfig.debug file:

config HAVE_SEMAPHORE_DEBUGGING
def_bool y

that was just off the top of my head, but it seems to work. more
generally, though, surely this sort of thing has come up before --
wanting to advertise an identical user-configurable option only for
certain architectures. how is that done *now* without rampant code
duplication? or is it?

rday

--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel
Pedantry Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================