Re: [RFC PATCH] Add TRACE_IRQFLAGS_SUPPORT, LOCKDEP_SUPPORT then

[Ingo Molnar <mingo@elte.hu>]

* Luming Yu <luming.yu@gmail.com> 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@hp.com>
> Signed-off-by: Yu Luming <luming.yu@intel.com>

> 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\x16,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@redhat.com>
> + */
> +#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