Re: [RFC PATCH] Add TRACE_IRQFLAGS_SUPPORT, LOCKDEP_SUPPORT thenenable ftrace for ia64

From: Ingo Molnar
Date: Fri Aug 28 2009 - 02:22:47 EST

Next message: Jan Beulich: "Re: [PATCH] x86: make use of inc/dec conditional"
Previous message: Peter Zijlstra: "Re: RFC: THE OFFLINE SCHEDULER"
In reply to: Luming Yu: "[RFC PATCH] Add TRACE_IRQFLAGS_SUPPORT, LOCKDEP_SUPPORT then enable ftrace for ia64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

* Luming Yu <luming.yu@xxxxxxxxx> wrote:

> Hi there,
>
> The following rfc patch is to add lockdep support and IRQ-flags
> state tracing support for ia64 architecture based on instructions
> described in irqflags-tracing.
>
> The next step is to fix issues. I plan to target it for -32 or
> -33.
>
> Ps. Bob Picco had done similar things before.
>
> Ps. The patch is enclosed in attachment. The inlined one is c&p of
> it for reading.
>
>
> Thanks,
> Luming
>
> Signed-off-by: Bob Picco <bob.picco@xxxxxx>
> Signed-off-by: Yu Luming <luming.yu@xxxxxxxxx>

> diff -BruN linux-2.6.31-rc6/arch/ia64/include/asm/page.h
> linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/page.h
> --- linux-2.6.31-rc6/arch/ia64/include/asm/page.h 2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/page.h 2009-08-23
> 18:59:00.000000000 -0700
> @@ -41,7 +41,7 @@
> #define PAGE_SIZE (__IA64_UL_CONST(1) << PAGE_SHIFT)
> #define PAGE_MASK (~(PAGE_SIZE - 1))
>
> -#define PERCPU_PAGE_SHIFT 16 /* log2() of max. size of per-CPU area */
> +#define PERCPU_PAGE_SHIFT 20 /*16 log2() of max. size of per-CPU area */
> #define PERCPU_PAGE_SIZE (__IA64_UL_CONST(1) << PERCPU_PAGE_SHIFT)

Why was this seemingly unrelated change done in a lockdep patch?

> diff -BruN linux-2.6.31-rc6/arch/ia64/include/asm/rwsem.h
> linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/rwsem.h
> --- linux-2.6.31-rc6/arch/ia64/include/asm/rwsem.h 2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/rwsem.h 2009-08-23
> 18:59:00.000000000 -0700
> @@ -37,6 +37,9 @@
> signed long count;
> spinlock_t wait_lock;
> struct list_head wait_list;
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + struct lockdep_map dep_map;
> +#endif
> };
>
> #define RWSEM_UNLOCKED_VALUE __IA64_UL_CONST(0x0000000000000000)
> diff -BruN linux-2.6.31-rc6/arch/ia64/include/asm/system.h
> linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/system.h
> --- linux-2.6.31-rc6/arch/ia64/include/asm/system.h 2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/include/asm/system.h 2009-08-23
> 18:59:00.000000000 -0700
> @@ -107,88 +107,6 @@
> */
> #define set_mb(var, value) do { (var) = (value); mb(); } while (0)
>
> -#define safe_halt() ia64_pal_halt_light() /* PAL_HALT_LIGHT */
> -
> -/*
> - * The group barrier in front of the rsm & ssm are necessary to ensure
> - * that none of the previous instructions in the same group are
> - * affected by the rsm/ssm.
> - */
> -/* For spinlocks etc */
> -
> -/*
> - * - clearing psr.i is implicitly serialized (visible by next insn)
> - * - setting psr.i requires data serialization
> - * - we need a stop-bit before reading PSR because we sometimes
> - * write a floating-point register right before reading the PSR
> - * and that writes to PSR.mfl
> - */
> -#ifdef CONFIG_PARAVIRT
> -#define __local_save_flags() ia64_get_psr_i()
> -#else
> -#define __local_save_flags() ia64_getreg(_IA64_REG_PSR)
> -#endif
> -
> -#define __local_irq_save(x) \
> -do { \
> - ia64_stop(); \
> - (x) = __local_save_flags(); \
> - ia64_stop(); \
> - ia64_rsm(IA64_PSR_I); \
> -} while (0)
> -
> -#define __local_irq_disable() \
> -do { \
> - ia64_stop(); \
> - ia64_rsm(IA64_PSR_I); \
> -} while (0)
> -
> -#define __local_irq_restore(x) ia64_intrin_local_irq_restore((x) & IA64_PSR_I)
> -
> -#ifdef CONFIG_IA64_DEBUG_IRQ
> -
> - extern unsigned long last_cli_ip;
> -
> -# define __save_ip() last_cli_ip = ia64_getreg(_IA64_REG_IP)
> -
> -# define local_irq_save(x) \
> -do { \
> - unsigned long __psr; \
> - \
> - __local_irq_save(__psr); \
> - if (__psr & IA64_PSR_I) \
> - __save_ip(); \
> - (x) = __psr; \
> -} while (0)
> -
> -# define local_irq_disable() do { unsigned long __x;
> local_irq_save(__x); } while (0)
> -
> -# define local_irq_restore(x) \
> -do { \
> - unsigned long __old_psr, __psr = (x); \
> - \
> - local_save_flags(__old_psr); \
> - __local_irq_restore(__psr); \
> - if ((__old_psr & IA64_PSR_I) && !(__psr & IA64_PSR_I)) \
> - __save_ip(); \
> -} while (0)
> -
> -#else /* !CONFIG_IA64_DEBUG_IRQ */
> -# define local_irq_save(x) __local_irq_save(x)
> -# define local_irq_disable() __local_irq_disable()
> -# define local_irq_restore(x) __local_irq_restore(x)
> -#endif /* !CONFIG_IA64_DEBUG_IRQ */
> -
> -#define local_irq_enable() ({ ia64_stop(); ia64_ssm(IA64_PSR_I);
> ia64_srlz_d(); })
> -#define local_save_flags(flags) ({ ia64_stop(); (flags) =
> __local_save_flags(); })
> -
> -#define irqs_disabled() \
> -({ \
> - unsigned long __ia64_id_flags; \
> - local_save_flags(__ia64_id_flags); \
> - (__ia64_id_flags & IA64_PSR_I) == 0; \
> -})
> -
> #ifdef __KERNEL__
>
> #ifdef CONFIG_IA32_SUPPORT
> @@ -274,7 +192,7 @@
> #define __ARCH_WANT_UNLOCKED_CTXSW
> #define ARCH_HAS_PREFETCH_SWITCH_STACK
> #define ia64_platform_is(x) (strcmp(x, platform_name) == 0)
> -
> +#include <linux/irqflags.h>
> void cpu_idle_wait(void);
>
> #define arch_align_stack(x) (x)
> diff -BruN linux-2.6.31-rc6/arch/ia64/Kconfig
> linux-2.6.31-rc6-lockdep/arch/ia64/Kconfig
> --- linux-2.6.31-rc6/arch/ia64/Kconfig 2009-08-13 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/Kconfig 2009-08-23
> 18:59:00.000000000 -0700
> @@ -143,6 +143,13 @@
>
> endif
>
> +config LOCKDEP_SUPPORT
> + bool
> + default y
> +
> +config STACKTRACE_SUPPORT
> + bool
> + default y

The (shorter) form we generally use when architectures enable a
feature is:

config LOCKDEP_SUPPORT
def_bool y

config STACKTRACE_SUPPORT
def_bool y

( Separate cleanup patch: it might make sense to offer this in
generic code and just select a HAVE_LOCKDEP_SUPPORT flag. This
affects all lockdep architectures so should be handled
separately. )

> choice
> prompt "System type"
> default IA64_GENERIC
> diff -BruN linux-2.6.31-rc6/arch/ia64/Kconfig.debug
> linux-2.6.31-rc6-lockdep/arch/ia64/Kconfig.debug
> --- linux-2.6.31-rc6/arch/ia64/Kconfig.debug 2009-08-13 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/Kconfig.debug 2009-08-23
> 18:58:59.000000000 -0700
> @@ -2,6 +2,10 @@
>
> source "lib/Kconfig.debug"
>
> +config TRACE_IRQFLAGS_SUPPORT
> + bool
> + default y

(same comment as above.)

> +
> choice
> prompt "Physical memory granularity"
> default IA64_GRANULE_64MB
> diff -BruN linux-2.6.31-rc6/arch/ia64/kernel/ivt.S
> linux-2.6.31-rc6-lockdep/arch/ia64/kernel/ivt.S
> --- linux-2.6.31-rc6/arch/ia64/kernel/ivt.S 2009-08-13 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/kernel/ivt.S 2009-08-23
> 18:58:59.000000000 -0700
> @@ -1603,6 +1603,11 @@
> SAVE_REST
> ;;
> MCA_RECOVER_RANGE(interrupt)
> +#ifdef CONFIG_TRACE_IRQFLAGS
> + ;;
> + br.call.sptk.many rp=trace_hardirqs_off
> + ;;
> +#endif
> alloc r14=ar.pfs,0,0,2,0 // must be first in an insn group
> MOV_FROM_IVR(out0, r8) // pass cr.ivr as first arg
> add out1=16,sp // pass pointer to pt_regs as second arg
> diff -BruN linux-2.6.31-rc6/arch/ia64/kernel/Makefile
> linux-2.6.31-rc6-lockdep/arch/ia64/kernel/Makefile
> --- linux-2.6.31-rc6/arch/ia64/kernel/Makefile 2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/kernel/Makefile 2009-08-23
> 18:58:59.000000000 -0700
> @@ -40,6 +40,7 @@
> obj-$(CONFIG_PCI_MSI) += msi_ia64.o
> mca_recovery-y += mca_drv.o mca_drv_asm.o
> obj-$(CONFIG_IA64_MC_ERR_INJECT)+= err_inject.o
> + obj-$(CONFIG_STACKTRACE) += stacktrace.o
>
> obj-$(CONFIG_PARAVIRT) += paravirt.o paravirtentry.o \
> paravirt_patch.o
> diff -BruN linux-2.6.31-rc6/arch/ia64/kernel/smpboot.c
> linux-2.6.31-rc6-lockdep/arch/ia64/kernel/smpboot.c
> --- linux-2.6.31-rc6/arch/ia64/kernel/smpboot.c 2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/kernel/smpboot.c 2009-08-23
> 18:58:59.000000000 -0700
> @@ -504,7 +504,7 @@
> struct create_idle c_idle = {
> .work = __WORK_INITIALIZER(c_idle.work, do_fork_idle),
> .cpu = cpu,
> - .done = COMPLETION_INITIALIZER(c_idle.done),
> + .done = COMPLETION_INITIALIZER_ONSTACK(c_idle.done),
> };
>
> c_idle.idle = get_idle_for_cpu(cpu);
> diff -BruN linux-2.6.31-rc6/arch/ia64/kernel/stacktrace.c
> linux-2.6.31-rc6-lockdep/arch/ia64/kernel/stacktrace.c
> --- linux-2.6.31-rc6/arch/ia64/kernel/stacktrace.c 1969-12-31
> 16:00:00.000000000 -0800
> +++ linux-2.6.31-rc6-lockdep/arch/ia64/kernel/stacktrace.c 2009-08-23
> 18:58:59.000000000 -0700
> @@ -0,0 +1,23 @@
> +/*
> + * arch/x86_64/kernel/stacktrace.c
> + *
> + * Stack trace management functions
> + *
> + * Copyright (C) 2006 Red Hat, Inc., Ingo Molnar <mingo@xxxxxxxxxx>
> + */
> +#include <linux/sched.h>
> +#include <linux/stacktrace.h>
> +#include <linux/module.h>
> +#include <asm/stacktrace.h>
> +
> +/*
> + * Save stack-backtrace addresses into a stack_trace buffer.
> + */
> +void save_stack_trace(struct stack_trace *trace)
> +{
> +}
> +EXPORT_SYMBOL(save_stack_trace);
> +void __ia64_save_stack_nonlocal(struct stack_trace *trace)

(nit: missing newline)

> +{
> +}
> +EXPORT_SYMBOL(__ia64_save_stack_nonlocal);

also, these functions should be implemented, for lockdep reports to
be readable. Generally this is done by librarizing the dump_stack()
et al architecture code into stacktrace.c.

> diff -BruN linux-2.6.31-rc6/include/asm-ia64/irqflags.h
> linux-2.6.31-rc6-lockdep/include/asm-ia64/irqflags.h
> --- linux-2.6.31-rc6/include/asm-ia64/irqflags.h 1969-12-31
> 16:00:00.000000000 -0800
> +++ linux-2.6.31-rc6-lockdep/include/asm-ia64/irqflags.h 2009-08-23
> 18:59:14.000000000 -0700
> @@ -0,0 +1,92 @@
> +#ifndef _ASM_IRQFLAGS_H
> +#define _ASM_IRQFLAGS_H
> +#include <asm/kregs.h>
> +#include <asm/pal.h>
> +/*
> + * The group barrier in front of the rsm & ssm are necessary to ensure
> + * that none of the previous instructions in the same group are
> + * affected by the rsm/ssm.
> + */
> +/* For spinlocks etc */
> +

( nit: looks a bit disorganized - could be merged into a single
comment block? )

> +/*
> + * - clearing psr.i is implicitly serialized (visible by next insn)
> + * - setting psr.i requires data serialization
> + * - we need a stop-bit before reading PSR because we sometimes
> + * write a floating-point register right before reading the PSR
> + * and that writes to PSR.mfl
> + */
> +#define __local_irq_save(x) \
> +do { \
> + ia64_stop(); \
> + (x) = ia64_getreg(_IA64_REG_PSR); \
> + ia64_stop(); \
> + ia64_rsm(IA64_PSR_I); \
> +} while (0)

please use C inline functions for all of irqflags.h (x86 does that
too). Macros have all sorts of disadvantages: they are harder to
read and also double evaluation side-effects are harder to keep
under control.

> +
> +#define __local_irq_disable() \
> +do { \
> + ia64_stop(); \
> + ia64_rsm(IA64_PSR_I); \
> +} while (0)
> +
> +#define __local_irq_restore(x) ia64_intrin_local_irq_restore((x) & IA64_PSR_I)
> +
> +#ifdef CONFIG_IA64_DEBUG_IRQ
> +
> + extern unsigned long last_cli_ip;
> +
> +# define __save_ip() last_cli_ip = ia64_getreg(_IA64_REG_IP)
> +
> +# define raw_local_irq_save(x) \
> +do { \
> + unsigned long psr; \
> + \
> + __local_irq_save(psr); \
> + if (psr & IA64_PSR_I) \
> + __save_ip(); \
> + (x) = psr; \
> +} while (0)
> +
> +# define raw_local_irq_disable() do { unsigned long x;
> raw_local_irq_save(x); } while (0)

( the patch seems line-wrapped, see Documentation/email-clients.txt
about how to send plain-text patches. )

> +
> +# define raw_local_irq_restore(x) \
> +do { \
> + unsigned long old_psr, psr = (x); \
> + \
> + local_save_flags(old_psr); \
> + __local_irq_restore(psr); \
> + if ((old_psr & IA64_PSR_I) && !(psr & IA64_PSR_I)) \
> + __save_ip(); \
> +} while (0)
> +
> +#else /* !CONFIG_IA64_DEBUG_IRQ */
> +# define raw_local_irq_save(x) __local_irq_save(x)
> +# define raw_local_irq_disable() __local_irq_disable()
> +# define raw_local_irq_restore(x) __local_irq_restore(x)
> +#endif /* !CONFIG_IA64_DEBUG_IRQ */
> +
> +#define raw_local_irq_enable() ({ ia64_stop(); ia64_ssm(IA64_PSR_I);
> ia64_srlz_d(); })
> +#define raw_local_save_flags(flags) ({ ia64_stop(); (flags) =
> ia64_getreg(_IA64_REG_PSR); })
> +
> +#define raw_irqs_disabled() \
> +({ \
> + unsigned long __ia64_id_flags; \
> + raw_local_save_flags(__ia64_id_flags); \
> + (__ia64_id_flags & IA64_PSR_I) == 0; \
> +})
> +
> +#define raw_safe_halt() ia64_pal_halt_light() /* PAL_HALT_LIGHT */
> +#define raw_irqs_disabled_flags(flags) \
> +({ \
> + (int)((flags) & IA64_PSR_I) == 0; \
> +})
> +
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +#define TRACE_IRQS_ON br.call.sptk.many b0=trace_hardirqs_on
> +#define TRACE_IRQS_OFF br.call.sptk.many b0=trace_hardirqs_off
> +#else
> +#define TRACE_IRQS_ON
> +#define TRACE_IRQS_OFF
> +#endif
> +#endif
> diff -BruN linux-2.6.31-rc6/include/asm-ia64/stacktrace.h
> linux-2.6.31-rc6-lockdep/include/asm-ia64/stacktrace.h
> --- linux-2.6.31-rc6/include/asm-ia64/stacktrace.h 1969-12-31
> 16:00:00.000000000 -0800
> +++ linux-2.6.31-rc6-lockdep/include/asm-ia64/stacktrace.h 2009-08-23
> 18:59:14.000000000 -0700
> @@ -0,0 +1,7 @@
> +#ifndef _ASM_STACKTRACE_H
> +#define _ASM_STACKTRACE_H 1
> +
> +/* Generic stack tracer with callbacks */
> +
> +
> +#endif
>
> linux-2.6.31-rc6-lockdep/include/linux/lockdep.h
> --- linux-2.6.31-rc6/include/linux/lockdep.h 2009-08-13 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/include/linux/lockdep.h 2009-08-23
> 18:59:20.000000000 -0700
> @@ -162,7 +162,7 @@
> u64 chain_key;
> };
>
> -#define MAX_LOCKDEP_KEYS_BITS 13
> +#define MAX_LOCKDEP_KEYS_BITS 10

Why did you have to do this bit?

> linux-2.6.31-rc6-lockdep/lib/Kconfig.debug
> --- linux-2.6.31-rc6/lib/Kconfig.debug 2009-08-13 15:43:34.000000000 -0700
> +++ linux-2.6.31-rc6-lockdep/lib/Kconfig.debug 2009-08-23
> 18:59:06.000000000 -0700
> @@ -504,7 +504,7 @@
>
> config DEBUG_LOCKDEP
> bool "Lock dependency engine debugging"
> - depends on DEBUG_KERNEL && LOCKDEP
> + depends on DEBUG_KERNEL && LOCKDEP && !IA64
> help

That should be fixed (and this chunk will then not be needed) before
this is merged.

Also, a few general comments:

- There's no lockdep_sys_exit support implemented. (This callback
is needed to detect lock counts leaking to user-space. Just call
it in the return-from-syscall codepath(s).)

- There's no changes to exception code assembly AFAICS - such as
debug traps, special exceptions, etc. You should review all
places in the IA64 code where there's open-coded or
hardware-implicit enable-irqs or disable-irqs instructions, not
just the main local_irq_*() functions. A starting point would be:

git grep IA64_PSR_I arch/ia64/ | grep .S:

but there may be instructions and trap entries where there's
implicit irq-flags behavior - those need to be examined too.

- Do kprobes work with this patch?

- No NMI support AFAICS - all NMI codepaths should be exempted from
irqflags coverage.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jan Beulich: "Re: [PATCH] x86: make use of inc/dec conditional"
Previous message: Peter Zijlstra: "Re: RFC: THE OFFLINE SCHEDULER"
In reply to: Luming Yu: "[RFC PATCH] Add TRACE_IRQFLAGS_SUPPORT, LOCKDEP_SUPPORT then enable ftrace for ia64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]