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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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>

This reminds me that I need to dust off my series which fixes
local_irq_save() to not capture its variables by name.

I.e. it would be used as "flags = local_irq_save();" in the normal C way.

I'm fairly sure I can also improve the generated assembly.

> ---
> 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.

You've got mixed tabs/spaces here.

>  
>  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_irq_disable_ret();
>      _spin_lock(lock);
>  }

Is it sensible to do this way around?

By writing the trace record while interrupts are disabled, you do
prevent nesting in the general case (but not in NMIs/MCEs or the
irqsave() variants), but you increase the critical region while
interrupts are disabled.

Alternatively, writing the trace record outside of the critical region
would reduce the size of the region.  Interpretation logic already needs
to cope with nesting, so is one extra level of nesting in this corner
case a problem?

Does the logic cope with the fact that interrupt gates automatically
disable interrupts?

~Andrew

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