Re: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state

From: Nicholas Piggin
Date: Wed Aug 12 2020 - 04:18:38 EST


Excerpts from peterz@xxxxxxxxxxxxx's message of August 7, 2020 9:11 pm:
>
> What's wrong with something like this?
>
> AFAICT there's no reason to actually try and add IRQ tracing here, it's
> just a hand full of instructions at the most.

Because we may want to use that in other places as well, so it would
be nice to have tracing.

Hmm... also, I thought NMI context was free to call local_irq_save/restore
anyway so the bug would still be there in those cases?

Thanks,
Nick

>
> ---
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 3a0db7b0b46e..6be22c1838e2 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -196,33 +196,6 @@ static inline bool arch_irqs_disabled(void)
> arch_local_irq_restore(flags); \
> } while(0)
>
> -#ifdef CONFIG_TRACE_IRQFLAGS
> -#define powerpc_local_irq_pmu_save(flags) \
> - do { \
> - raw_local_irq_pmu_save(flags); \
> - trace_hardirqs_off(); \
> - } while(0)
> -#define powerpc_local_irq_pmu_restore(flags) \
> - do { \
> - if (raw_irqs_disabled_flags(flags)) { \
> - raw_local_irq_pmu_restore(flags); \
> - trace_hardirqs_off(); \
> - } else { \
> - trace_hardirqs_on(); \
> - raw_local_irq_pmu_restore(flags); \
> - } \
> - } while(0)
> -#else
> -#define powerpc_local_irq_pmu_save(flags) \
> - do { \
> - raw_local_irq_pmu_save(flags); \
> - } while(0)
> -#define powerpc_local_irq_pmu_restore(flags) \
> - do { \
> - raw_local_irq_pmu_restore(flags); \
> - } while (0)
> -#endif /* CONFIG_TRACE_IRQFLAGS */
> -
> #endif /* CONFIG_PPC_BOOK3S */
>
> #ifdef CONFIG_PPC_BOOK3E
> diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h
> index bc4bd19b7fc2..b357a35672b1 100644
> --- a/arch/powerpc/include/asm/local.h
> +++ b/arch/powerpc/include/asm/local.h
> @@ -32,9 +32,9 @@ static __inline__ void local_##op(long i, local_t *l) \
> { \
> unsigned long flags; \
> \
> - powerpc_local_irq_pmu_save(flags); \
> + raw_powerpc_local_irq_pmu_save(flags); \
> l->v c_op i; \
> - powerpc_local_irq_pmu_restore(flags); \
> + raw_powerpc_local_irq_pmu_restore(flags); \
> }
>
> #define LOCAL_OP_RETURN(op, c_op) \
> @@ -43,9 +43,9 @@ static __inline__ long local_##op##_return(long a, local_t *l) \
> long t; \
> unsigned long flags; \
> \
> - powerpc_local_irq_pmu_save(flags); \
> + raw_powerpc_local_irq_pmu_save(flags); \
> t = (l->v c_op a); \
> - powerpc_local_irq_pmu_restore(flags); \
> + raw_powerpc_local_irq_pmu_restore(flags); \
> \
> return t; \
> }
> @@ -81,11 +81,11 @@ static __inline__ long local_cmpxchg(local_t *l, long o, long n)
> long t;
> unsigned long flags;
>
> - powerpc_local_irq_pmu_save(flags);
> + raw_powerpc_local_irq_pmu_save(flags);
> t = l->v;
> if (t == o)
> l->v = n;
> - powerpc_local_irq_pmu_restore(flags);
> + raw_powerpc_local_irq_pmu_restore(flags);
>
> return t;
> }
> @@ -95,10 +95,10 @@ static __inline__ long local_xchg(local_t *l, long n)
> long t;
> unsigned long flags;
>
> - powerpc_local_irq_pmu_save(flags);
> + raw_powerpc_local_irq_pmu_save(flags);
> t = l->v;
> l->v = n;
> - powerpc_local_irq_pmu_restore(flags);
> + raw_powerpc_local_irq_pmu_restore(flags);
>
> return t;
> }
> @@ -117,12 +117,12 @@ static __inline__ int local_add_unless(local_t *l, long a, long u)
> unsigned long flags;
> int ret = 0;
>
> - powerpc_local_irq_pmu_save(flags);
> + raw_powerpc_local_irq_pmu_save(flags);
> if (l->v != u) {
> l->v += a;
> ret = 1;
> }
> - powerpc_local_irq_pmu_restore(flags);
> + raw_powerpc_local_irq_pmu_restore(flags);
>
> return ret;
> }
>