Re: [tip: locking/core] lockdep: Annotate irq_work

[Frederic Weisbecker]

On Sat, Mar 21, 2020 at 03:53:45PM -0000, tip-bot2 for Sebastian Andrzej Siewior wrote:
> The following commit has been merged into the locking/core branch of tip:
> 
> Commit-ID:     49915ac35ca7b07c54295a72d905be5064afb89e
> Gitweb:        https://git.kernel.org/tip/49915ac35ca7b07c54295a72d905be5064afb89e
> Author:        Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> AuthorDate:    Sat, 21 Mar 2020 12:26:03 +01:00
> Committer:     Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> CommitterDate: Sat, 21 Mar 2020 16:00:24 +01:00
> 
> lockdep: Annotate irq_work
> 
> Mark irq_work items with IRQ_WORK_HARD_IRQ which should be invoked in
> hardirq context even on PREEMPT_RT. IRQ_WORK without this flag will be
> invoked in softirq context on PREEMPT_RT.
> 
> Set ->irq_config to 1 for the IRQ_WORK items which are invoked in softirq
> context so lockdep knows that these can safely acquire a spinlock_t.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Link: https://lkml.kernel.org/r/20200321113242.643576700@xxxxxxxxxxxxx
> ---
>  include/linux/irq_work.h |  2 ++
>  include/linux/irqflags.h | 13 +++++++++++++
>  kernel/irq_work.c        |  2 ++
>  kernel/rcu/tree.c        |  1 +
>  kernel/time/tick-sched.c |  1 +
>  5 files changed, 19 insertions(+)
> 
> diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
> index 02da997..3b752e8 100644
> --- a/include/linux/irq_work.h
> +++ b/include/linux/irq_work.h
> @@ -18,6 +18,8 @@
>  
>  /* Doesn't want IPI, wait for tick: */
>  #define IRQ_WORK_LAZY		BIT(2)
> +/* Run hard IRQ context, even on RT */
> +#define IRQ_WORK_HARD_IRQ	BIT(3)
>  
>  #define IRQ_WORK_CLAIMED	(IRQ_WORK_PENDING | IRQ_WORK_BUSY)
>  
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 9c17f9c..f23f540 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -69,6 +69,17 @@ do {						\
>  			current->irq_config = 0;	\
>  	  } while (0)
>  
> +# define lockdep_irq_work_enter(__work)					\
> +	  do {								\
> +		  if (!(atomic_read(&__work->flags) & IRQ_WORK_HARD_IRQ))\
> +			current->irq_config = 1;			\

So, irq_config == 1 means we are in a softirq? Are there other values for
irq_config? In which case there should be enums or something?
I can't find the patch that describes this.

> +	  } while (0)
> +# define lockdep_irq_work_exit(__work)					\
> +	  do {								\
> +		  if (!(atomic_read(&__work->flags) & IRQ_WORK_HARD_IRQ))\
> +			current->irq_config = 0;			\
> +	  } while (0)
> +
>  #else
>  # define trace_hardirqs_on()		do { } while (0)
>  # define trace_hardirqs_off()		do { } while (0)
> @@ -83,6 +94,8 @@ do {						\
>  # define lockdep_softirq_exit()		do { } while (0)
>  # define lockdep_hrtimer_enter(__hrtimer)		do { } while (0)
>  # define lockdep_hrtimer_exit(__hrtimer)		do { } while (0)
> +# define lockdep_irq_work_enter(__work)		do { } while (0)
> +# define lockdep_irq_work_exit(__work)		do { } while (0)
>  #endif
>  
>  #if defined(CONFIG_IRQSOFF_TRACER) || \
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index 828cc30..48b5d1b 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -153,7 +153,9 @@ static void irq_work_run_list(struct llist_head *list)
>  		 */
>  		flags = atomic_fetch_andnot(IRQ_WORK_PENDING, &work->flags);
>  
> +		lockdep_irq_work_enter(work);
>  		work->func(work);
> +		lockdep_irq_work_exit(work);
>  		/*
>  		 * Clear the BUSY bit and return to the free state if
>  		 * no-one else claimed it meanwhile.
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d91c915..5066d1d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1113,6 +1113,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
>  		    !rdp->rcu_iw_pending && rdp->rcu_iw_gp_seq != rnp->gp_seq &&
>  		    (rnp->ffmask & rdp->grpmask)) {
>  			init_irq_work(&rdp->rcu_iw, rcu_iw_handler);
> +			atomic_set(&rdp->rcu_iw.flags, IRQ_WORK_HARD_IRQ);
>  			rdp->rcu_iw_pending = true;
>  			rdp->rcu_iw_gp_seq = rnp->gp_seq;
>  			irq_work_queue_on(&rdp->rcu_iw, rdp->cpu);
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 4be756b..3e2dc9b 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -245,6 +245,7 @@ static void nohz_full_kick_func(struct irq_work *work)
>  
>  static DEFINE_PER_CPU(struct irq_work, nohz_full_kick_work) = {
>  	.func = nohz_full_kick_func,
> +	.flags = ATOMIC_INIT(IRQ_WORK_HARD_IRQ),
>  };

I get why these need to be in hardirq but some basic explanations for
ordinary mortals as to why those two specifically and not all the others
(and there are many) would have been nice.

Thanks.

>  
>  /*