[v1] enhance lock debugging

[Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Juergen Gross 6 years, 6 months ago

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.
+
 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 {
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Andrew Cooper 6 years, 6 months ago

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>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Juergen Gross 6 years, 6 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Jan Beulich 6 years, 6 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Juergen Gross 6 years, 6 months ago

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Jan Beulich 6 years, 6 months ago

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

Posted by Juergen Gross 6 years, 6 months ago

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel