mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH] sparc64: Fix irqtrace warnings on Ultra-S
@ 2020-09-08 16:12 Guenter Roeck
  0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2020-09-08 16:12 UTC (permalink / raw)
  To: peterz
  Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
	rostedt, rjw, joel, svens, tglx, davem

On Tue, Sep 08, 2020 at 05:41:57PM +0200, peterz@infradead.org wrote:
> On Tue, Sep 08, 2020 at 07:40:23AM -0700, Guenter Roeck wrote:
> > qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
> >     -initrd rootfs.cpio \
> >     -kernel arch/sparc/boot/image -no-reboot \
> >     -append "panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0" \
> >     -nographic -monitor none
> 
> Thanks I got it. Also enabling DEBUG_LOCKDEP helps (-:
> 
> ---
> Subject: sparc64: Fix irqtrace warnings on Ultra-S
> 
> Recent changes in Lockdep's IRQTRACE broke Ultra-S.
> 
> In order avoid redundant IRQ state changes, local_irq_restore() lost the
> ability to trace a disable. Change the code to use local_irq_save() to
> disable IRQs and then use arch_local_irq_restore() to further disable
> NMIs.
> 
> This result in slightly suboptimal code, but given this code uses a
> global spinlock, performance cannot be its primary purpose.
> 
> Fixes: 044d0d6de9f5 ("lockdep: Only trace IRQ edges")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
> A possible alternative would be:
> 
> 	local_save_flags(flags);
> 	arch_local_irq_restore((unsigned long)PIL_NMI);
> 	if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS))
> 		trace_hardirqs_off();
> 
> which generates optimal code, but is more verbose.
> 
>  arch/sparc/prom/p1275.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
> index 889aa602f8d8..e22233fcf741 100644
> --- a/arch/sparc/prom/p1275.c
> +++ b/arch/sparc/prom/p1275.c
> @@ -37,16 +37,15 @@ void p1275_cmd_direct(unsigned long *args)
>  {
>  	unsigned long flags;
>  
> -	local_save_flags(flags);
> -	local_irq_restore((unsigned long)PIL_NMI);
> +	local_irq_save(flags);
> +	arch_local_irq_restore((unsigned long)PIL_NMI);
>  	raw_spin_lock(&prom_entry_lock);
>  
>  	prom_world(1);
>  	prom_cif_direct(args);
>  	prom_world(0);
>  
> -	raw_spin_unlock(&prom_entry_lock);
> -	local_irq_restore(flags);
> +	raw_spin_unlock_irqrestore(&prom_entry_lock, flags);
>  }
>  
>  void prom_cif_init(void *cif_handler, void *cif_stack)

^ permalink raw reply	[flat|nested] 2+ messages in thread
* [PATCH v2 00/11] TRACE_IRQFLAGS wreckage
@ 2020-08-21  8:47 Peter Zijlstra
  2020-08-21  8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
  0 siblings, 1 reply; 2+ messages in thread
From: Peter Zijlstra @ 2020-08-21  8:47 UTC (permalink / raw)
  To: linux-kernel, mingo, will
  Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz


TRACE_IRQFLAGS

local_irq_*() keeps a software state that mirrors the hardware state,
used for lockdep, includes tracepoints.

raw_local_irq_*() does not update the software state, no tracing.

---

Problem 1:

	raw_local_irq_save(); // software state on
	local_irq_save(); // software state off
	...
	local_irq_restore(); // software state still off, because we don't enable IRQs
	raw_local_irq_restore(); // software state still off, *whoopsie*

existing instances:

 - lock_acquire()
     raw_local_irq_save()
     __lock_acquire()
       arch_spin_lock(&graph_lock)
         pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
           local_irq_save()

 - trace_clock_global()
     raw_local_irq_save()
     arch_spin_lock()
       pv_wait() := kvm_wait()
	 local_irq_save()

 - apic_retrigger_irq()
     raw_local_irq_save()
     apic->send_IPI() := default_send_IPI_single_phys()
       local_irq_save()

Possible solutions:

 A) make it work by enabling the tracing inside raw_*()
 B) make it work by keeping tracing disabled inside raw_*()
 C) call it broken and clean it up now

Now, given that the only reason to use the raw_* variant is because you don't
want tracing. Therefore A) seems like a weird option (although it can be done).
C) is tempting, but OTOH it ends up converting a _lot_ of code to raw just
because there is one raw user, this strips the validation/tracing off for all
the other users.

So we pick B) and declare any code that ends up doing:

	raw_local_irq_save()
	local_irq_save()
	lockdep_assert_irqs_disabled();

broken. AFAICT this problem has existed forever, the only reason it came
up is because I changed IRQ tracing vs lockdep recursion and the first
instance is fairly common, the other cases hardly ever happen.

---

Problem 2:

	raw_local_irq_save(); // software state on
	trace_*()
          ...
          perf_tp_event()
            ...
            perf_callchain()
	    <#PF>
	      trace_hardirqs_off(); // software state off
	      ...
	      if (regs_irqs_disabled(regs)) // false
	        trace_hardirqs_on();
	    </#PF>
	raw_local_irq_restore(); // software state stays off, *whoopsie*

existing instances:

 - lock_acquire() / lock_release()
     raw_local_irq_save()
     trace_lock_acquire() / trace_lock_release()

 - function tracing

Possible solutions:

 A) fix every architecture's entry code
 B) only fix kernel/entry/common.c
 C) fix lockdep tracepoints and pray

This series does C, AFAICT this problem has existed forever.

---

Problem 3:

	raw_local_irq_save(); // software state on
	<#NMI>
	  trace_hardirqs_off(); // software state off
	  ...
	  if (regs_irqs_disabled(regs)) // false
	    trace_hardirqs_on();
	</#NMI>
	raw_local_irq_restore(); // software state stays off, *whoopsie*

Possible solutions:

This *should* not be a problem if an architecture has it's entry ordering
right. In particular we rely on the architecture doing nmi_enter() before
trace_hardirqs_off().

In that case, in_nmi() will be true, and lockdep_hardirqs_*() should NO-OP,
except if CONFIG_TRACE_IRQFLAGS_NMI (x86).

There might be a problem with using lockdep_assert_irqs_disabled() from NMI
context, if so, those needs a little TLC.

---

The patches in this series do (in reverse order):

 - 2C
 - 1B
 - fix fallout in idle due to the trace_lock_*() tracepoints suddenly
   being visible to rcu-lockdep.

---

Change since -v1:

 - typo (rostedt)
 - split WARN (rostedt)
 - reorder start_critical_section / rcu_idle_enter (rostedt)
 - added arm64 patch (kernel test robot)

---

 arch/arm/mach-omap2/pm34xx.c      |    4 --
 arch/arm64/include/asm/irqflags.h |    5 ++
 arch/arm64/kernel/process.c       |    2 -
 arch/nds32/include/asm/irqflags.h |    5 ++
 arch/powerpc/include/asm/hw_irq.h |   11 ++---
 arch/s390/kernel/idle.c           |    3 -
 arch/x86/entry/thunk_32.S         |    5 --
 arch/x86/include/asm/mmu.h        |    1 
 arch/x86/kernel/process.c         |    4 --
 arch/x86/mm/tlb.c                 |   13 +-----
 drivers/cpuidle/cpuidle.c         |   19 +++++++--
 drivers/idle/intel_idle.c         |   16 --------
 include/linux/cpuidle.h           |   13 +++---
 include/linux/irqflags.h          |   73 ++++++++++++++++++++------------------
 include/linux/lockdep.h           |   18 ++++++---
 include/linux/mmu_context.h       |    5 ++
 kernel/locking/lockdep.c          |   18 +++++----
 kernel/sched/idle.c               |   25 +++++--------
 18 files changed, 118 insertions(+), 122 deletions(-)



^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-09-08 19:43 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 16:12 [PATCH] sparc64: Fix irqtrace warnings on Ultra-S Guenter Roeck
  -- strict thread matches above, loose matches on Subject: below --
2020-08-21  8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
2020-08-21  8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
2020-09-02  4:21   ` Guenter Roeck
2020-09-02  9:09     ` peterz
2020-09-02  9:12       ` peterz
2020-09-02 13:48         ` Guenter Roeck
2020-09-08 14:22           ` peterz
2020-09-08 14:40             ` Guenter Roeck
2020-09-08 15:41               ` [PATCH] sparc64: Fix irqtrace warnings on Ultra-S peterz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®