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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.06.17 at 19:34, <dario.faggioli@xxxxxxxxxx> wrote:
> @@ -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

I'm not aware of any behavioral difference between this an not
specifying a default at all, so I'd like to ask to follow suit (see
surrounding code) and omit this line.

> --- 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();

Hmm, looks you're introducing new non-static, underscore-prefixed
symbols. Please don't, use e.g. a "arch_" prefix or a "_notrace" suffix
instead.

> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -5,6 +5,8 @@
>  #include <xen/bitops.h>
>  #include <asm/processor.h>
>  
> +#include <xen/trace.h>

Why?

> @@ -214,6 +216,79 @@ static always_inline unsigned long __xadd(
>                         "ri" ( (x) & X86_EFLAGS_IF ) );           \
>  })
>  
> +#ifdef CONFIG_TRACE_IRQSOFF
> +
> +#define TRACE_LOCAL_ADDR ((uint64_t) current_text_addr())
> +#define TRACE_RET_ADDR   ((uint64_t) __builtin_return_address(0))
> +
> +#define trace_irq_disable(_a)                                    \
> +({                                                               \
> +    uint64_t addr = _a;                                          \
> +    __trace_var(TRC_HW_IRQ_DISABLE, 1, sizeof(addr), &addr);     \
> +})
> +#define trace_irq_enable(_a)                                     \
> +({                                                               \
> +    uint64_t addr = _a;                                          \
> +    __trace_var(TRC_HW_IRQ_ENABLE, 1, sizeof(addr), &addr);      \
> +})
> +#define trace_irq_save(_x, _a)                                   \
> +({                                                               \
> +    uint64_t addr = _a;                                          \
> +    if ( _x & X86_EFLAGS_IF )                                    \
> +        __trace_var(TRC_HW_IRQ_DISABLE, 1, sizeof(addr), &addr); \
> +})
> +#define trace_irq_restore(_x, _a)                                \
> +({                                                               \
> +    uint64_t addr = _a;                                          \
> +    if ( _x & X86_EFLAGS_IF )                                    \
> +        __trace_var(TRC_HW_IRQ_ENABLE, 1, sizeof(addr), &addr);  \
> +})
> +
> +#define trace_irq_disable_ret()   trace_irq_disable(TRACE_RET_ADDR)
> +#define trace_irq_enable_ret()    trace_irq_enable(TRACE_RET_ADDR)
> +#define trace_irq_save_ret(_x)    trace_irq_save(_x, TRACE_RET_ADDR)
> +#define trace_irq_restore_ret(_x) trace_irq_restore(_x, TRACE_RET_ADDR)
> +
> +#define local_irq_disable()                      \
> +({                                               \
> +    bool_t irqon = local_irq_is_enabled();       \
> +    _local_irq_disable();                        \
> +    if ( unlikely(tb_init_done && irqon) )       \
> +        trace_irq_disable(TRACE_LOCAL_ADDR);     \
> +})
> +
> +#define local_irq_enable()                       \
> +({                                               \
> +    if ( unlikely(tb_init_done) )                \
> +        trace_irq_enable(TRACE_LOCAL_ADDR);      \
> +    _local_irq_enable();                         \
> +})
> +
> +#define local_irq_save(_x)                       \
> +({                                               \
> +    local_save_flags(_x);                        \
> +    _local_irq_disable();                        \
> +    if ( unlikely(tb_init_done) )                \
> +        trace_irq_save(_x, TRACE_LOCAL_ADDR);    \
> +})
> +
> +#define local_irq_restore(_x)                    \
> +({                                               \
> +    if ( unlikely(tb_init_done) )                \
> +        trace_irq_restore(_x, TRACE_LOCAL_ADDR); \
> +    _local_irq_restore(_x);                      \
> +})
> +#else /* !TRACE_IRQSOFF */
> +#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)
> +#endif /* TRACE_IRQSOFF */

None of this looks to be arch-specific.

Jan

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