| Message ID | 20190807143119.8360-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enhance lock debugging | expand |
On 07/08/2019 15:31, Juergen Gross wrote: > Instead of enabling spinlock debugging for debug builds only add a > dedicated Kconfig option for that purpose which defaults to DEBUG. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > xen/Kconfig.debug | 7 +++++++ > xen/common/spinlock.c | 4 ++-- > xen/include/xen/spinlock.h | 2 +- > 3 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug > index e10e314e25..5db3e7ec25 100644 > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -44,6 +44,13 @@ config COVERAGE > > If unsure, say N here. > > +config SPINLOCK_DEBUG > + bool "Spinlock debugging" > + default DEBUG > + ---help--- > + Enable debugging features of spinlock handling. Some additional > + checks will be performed when acquiring and releasing locks. Missing tab on the final line. Can be fixed on commit). Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 07.08.19 19:17, Andrew Cooper wrote: > On 07/08/2019 15:31, Juergen Gross wrote: >> Instead of enabling spinlock debugging for debug builds only add a >> dedicated Kconfig option for that purpose which defaults to DEBUG. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> xen/Kconfig.debug | 7 +++++++ >> xen/common/spinlock.c | 4 ++-- >> xen/include/xen/spinlock.h | 2 +- >> 3 files changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug >> index e10e314e25..5db3e7ec25 100644 >> --- a/xen/Kconfig.debug >> +++ b/xen/Kconfig.debug >> @@ -44,6 +44,13 @@ config COVERAGE >> >> If unsure, say N here. >> >> +config SPINLOCK_DEBUG >> + bool "Spinlock debugging" >> + default DEBUG >> + ---help--- >> + Enable debugging features of spinlock handling. Some additional >> + checks will be performed when acquiring and releasing locks. > > Missing tab on the final line. Can be fixed on commit). Oh, sorry. Will fix in V2. > > Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks, Juergen
On 07.08.2019 16:31, Juergen Gross wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -44,6 +44,13 @@ config COVERAGE > > If unsure, say N here. > > +config SPINLOCK_DEBUG > + bool "Spinlock debugging" > + default DEBUG > + ---help--- > + Enable debugging features of spinlock handling. Some additional > + checks will be performed when acquiring and releasing locks. > + > config LOCK_PROFILE While the pre-existing LOCK_PROFILE suggests the opposite, I'd still like to propose that we uniformly name all debugging options CONFIG_DEBUG_* (rather than having the DEBUG at the end). Thoughts? Jan
On 08.08.19 08:34, Jan Beulich wrote: > On 07.08.2019 16:31, Juergen Gross wrote: >> --- a/xen/Kconfig.debug >> +++ b/xen/Kconfig.debug >> @@ -44,6 +44,13 @@ config COVERAGE >> If unsure, say N here. >> +config SPINLOCK_DEBUG >> + bool "Spinlock debugging" >> + default DEBUG >> + ---help--- >> + Enable debugging features of spinlock handling. Some additional >> + checks will be performed when acquiring and releasing locks. >> + >> config LOCK_PROFILE > > While the pre-existing LOCK_PROFILE suggests the opposite, I'd > still like to propose that we uniformly name all debugging > options CONFIG_DEBUG_* (rather than having the DEBUG at the > end). Thoughts? Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm touching it anyway. Juergen
On 08.08.2019 09:23, Juergen Gross wrote: > On 08.08.19 08:34, Jan Beulich wrote: >> On 07.08.2019 16:31, Juergen Gross wrote: >>> --- a/xen/Kconfig.debug >>> +++ b/xen/Kconfig.debug >>> @@ -44,6 +44,13 @@ config COVERAGE >>> If unsure, say N here. >>> +config SPINLOCK_DEBUG >>> + bool "Spinlock debugging" >>> + default DEBUG >>> + ---help--- >>> + Enable debugging features of spinlock handling. Some additional >>> + checks will be performed when acquiring and releasing locks. >>> + >>> config LOCK_PROFILE >> >> While the pre-existing LOCK_PROFILE suggests the opposite, I'd >> still like to propose that we uniformly name all debugging >> options CONFIG_DEBUG_* (rather than having the DEBUG at the >> end). Thoughts? > > Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm > touching it anyway. One more thing: Perhaps it would better be DEBUG_LOCK (i.e. without "SPIN") or DEBUG_LOCKS, to also allow it to cover r/w locks, should anyone want to instrument them as well. Jan
On 08.08.19 09:35, Jan Beulich wrote: > On 08.08.2019 09:23, Juergen Gross wrote: >> On 08.08.19 08:34, Jan Beulich wrote: >>> On 07.08.2019 16:31, Juergen Gross wrote: >>>> --- a/xen/Kconfig.debug >>>> +++ b/xen/Kconfig.debug >>>> @@ -44,6 +44,13 @@ config COVERAGE >>>> If unsure, say N here. >>>> +config SPINLOCK_DEBUG >>>> + bool "Spinlock debugging" >>>> + default DEBUG >>>> + ---help--- >>>> + Enable debugging features of spinlock handling. Some additional >>>> + checks will be performed when acquiring and releasing locks. >>>> + >>>> config LOCK_PROFILE >>> >>> While the pre-existing LOCK_PROFILE suggests the opposite, I'd >>> still like to propose that we uniformly name all debugging >>> options CONFIG_DEBUG_* (rather than having the DEBUG at the >>> end). Thoughts? >> >> Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm >> touching it anyway. > > One more thing: Perhaps it would better be DEBUG_LOCK (i.e. > without "SPIN") or DEBUG_LOCKS, to also allow it to cover r/w > locks, should anyone want to instrument them as well. Yes. I'll switch to DEBUG_LOCKS. Juergen
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index e10e314e25..5db3e7ec25 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -44,6 +44,13 @@ config COVERAGE If unsure, say N here. +config SPINLOCK_DEBUG + bool "Spinlock debugging" + default DEBUG + ---help--- + Enable debugging features of spinlock handling. Some additional + checks will be performed when acquiring and releasing locks. + config LOCK_PROFILE bool "Lock Profiling" ---help--- diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 4e681cc651..16356ec9b7 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -9,7 +9,7 @@ #include <asm/processor.h> #include <asm/atomic.h> -#ifndef NDEBUG +#ifdef CONFIG_SPINLOCK_DEBUG static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0); @@ -97,7 +97,7 @@ void spin_debug_disable(void) atomic_dec(&spin_debug); } -#else /* defined(NDEBUG) */ +#else /* CONFIG_SPINLOCK_DEBUG */ #define check_lock(l) ((void)0) #define check_barrier(l) ((void)0) diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h index b4881ad433..e3458f10dd 100644 --- a/xen/include/xen/spinlock.h +++ b/xen/include/xen/spinlock.h @@ -6,7 +6,7 @@ #include <asm/types.h> #include <xen/percpu.h> -#ifndef NDEBUG +#ifdef CONFIG_SPINLOCK_DEBUG union lock_debug { unsigned short val; struct {
Instead of enabling spinlock debugging for debug builds only add a dedicated Kconfig option for that purpose which defaults to DEBUG. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/Kconfig.debug | 7 +++++++ xen/common/spinlock.c | 4 ++-- xen/include/xen/spinlock.h | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-)