Re: [PATCH 06/15] xen: trace IRQ enabling/disabling

[Julien Grall <julien.grall@arm.com>]

Hi Dario,

On 01/06/17 18:34, Dario Faggioli wrote:
> Trace when interrupts are disabled and (re)enabled.
> Basically, we replace the IRQ disabling and enabling
> functions with helpers that does the same, but also
> output the proper trace record.
>
> For putting in the record something that will let
> us identify _where_ in the code (i.e., in what function)
> the IRQ manipulation is happening, use either:
>  - current_text_addr(),
>  - or __builtin_return_address(0).
>
> In fact, depending on whether the disabling/enabling
> happens in macros (like for local_irq_disable() and
> local_irq_enable()) or in actual functions (like in
> spin_lock_irq*()), it is either:
>  - the actual content of the instruction pointer when
>    IRQ are disabled/enabled,
>  - or the return address of the utility function where
>    IRQ are disabled/enabled,
> that will tell us what it is the actual piece of code
> that is asking for the IRQ manipulation operation.
>
> Gate this with its specific Kconfig option, and keep
> it in disabled state by default (i.e., don't build it,
> if not explicitly specified), as the impact on
> performance may be non negligible.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>
> ---
>  xen/Kconfig.debug                  |   11 ++++-
>  xen/common/spinlock.c              |   16 +++++--
>  xen/common/trace.c                 |   10 +++-
>  xen/include/asm-arm/arm32/system.h |   12 +++++
>  xen/include/asm-arm/arm64/system.h |   12 +++++
>  xen/include/asm-x86/system.h       |   85 ++++++++++++++++++++++++++++++++++--
>  xen/include/public/trace.h         |    2 +
>  xen/include/xen/rwlock.h           |   33 +++++++++++---
>  8 files changed, 161 insertions(+), 20 deletions(-)
>
> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> index 374c1c0..81910c9 100644
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -98,7 +98,7 @@ config PERF_ARRAYS
>  	---help---
>  	  Enables software performance counter array histograms.
>
> -config TRACING
> +menuconfig TRACING
>  	bool "Tracing"
>  	default y
>  	---help---
> @@ -106,6 +106,15 @@ config TRACING
>  	  in per-CPU ring buffers. The 'xentrace' tool can be used to read
>  	  the buffers and dump the content on the disk.
>
> +config TRACE_IRQSOFF
> +	bool "Trace when IRQs are disabled and (re)enabled" if EXPERT = "y"
> +	default n
> +	depends on TRACING
> +	---help---
> +	  Makes it possible to generate events _every_ time IRQs are disabled
> +          and (re)enabled, with also an indication of where that happened.
> +          Note that this comes with high overead and produces huge amount of
> +          tracing data.
>
>  config VERBOSE_DEBUG
>  	bool "Verbose debug messages"
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 2a06406..33b903e 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -150,7 +150,9 @@ void _spin_lock(spinlock_t *lock)
>  void _spin_lock_irq(spinlock_t *lock)
>  {
>      ASSERT(local_irq_is_enabled());
> -    local_irq_disable();
> +    _local_irq_disable();
> +    if ( unlikely(tb_init_done) )

__trace_var already contain a "if ( !tb_init_done )". It sounds 
pointless to do it twice, and also I think it is not obvious to 
understand the meaning of the check (i.e what is tb_init_done).

Would it be possible to hide this check and avoid check tb_init_done twice?

> diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
> index c617b40..20871ad 100644
> --- a/xen/include/asm-arm/arm32/system.h
> +++ b/xen/include/asm-arm/arm32/system.h
> @@ -4,6 +4,8 @@
>
>  #include <asm/arm32/cmpxchg.h>
>
> +#include <xen/trace.h>
> +
>  #define local_irq_disable() asm volatile ( "cpsid i @ local_irq_disable\n" : : : "cc" )
>  #define local_irq_enable()  asm volatile ( "cpsie i @ local_irq_enable\n" : : : "cc" )
>
> @@ -41,6 +43,16 @@ static inline int local_irq_is_enabled(void)
>  #define local_abort_enable() __asm__("cpsie a  @ __sta\n" : : : "memory", "cc")
>  #define local_abort_disable() __asm__("cpsid a @ __sta\n" : : : "memory", "cc")
>
> +/* We do not support tracing (at all) yet */

I know that some bits are missing for ARM, but the patch #5 contradicts 
this comment as you have CONFIG_TRACE=y by default.

> +#define trace_irq_disable_ret()   do { } while ( 0 )
> +#define trace_irq_enable_ret()    do { } while ( 0 )
> +#define trace_irq_save_ret(_x)    do { } while ( 0 )
> +#define trace_irq_restore_ret(_x) do { } while ( 0 )
> +#define _local_irq_disable()      local_irq_disable()
> +#define _local_irq_enable()       local_irq_enable()
> +#define _local_irq_save(_x)       local_irq_save(_x)
> +#define _local_irq_restore(_x)    local_irq_restore(_x)
> +

This does not need to be duplicated in both asm-arm/arm{32,64}/system.h. 
You could directly implement them in asm-arm/system.h.

Also, I would prefer if you stay consistent with x86. I.e non-underscore 
versions are calling the underscore versions and not the invert.

Cheers,

-- 
Julien Grall

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