[RFC PATCH 0/3] lockdep/irq: wait-type related cleanups

[RFC PATCH 0/3] lockdep/irq: wait-type related cleanups

[Frederic Weisbecker]

Hi,

Here are a few cleanups after reviewing the wait-type related changes.
It's RFC since I might not have guessed correctly all your aims.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	locking/core

HEAD: 2324f8c6891a858aa1c61933489ebc266282e67f

Thanks,
	Frederic
---

Frederic Weisbecker (3):
      lockdep/irq: Be more strict about IRQ-threadable code end
      lockdep: Merge hardirq_threaded and irq_config together
      lockdep: Briefly comment current->hardirq_threadable usecases


 include/linux/irqflags.h       | 80 +++++++++++++++++++-----------------------
 include/linux/sched.h          |  3 +-
 kernel/irq/handle.c            | 12 +++++--
 kernel/locking/lockdep.c       |  2 +-
 kernel/time/posix-cpu-timers.c |  7 ++--
 kernel/time/tick-sched.c       |  4 +++
 6 files changed, 56 insertions(+), 52 deletions(-)

[RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end

[Frederic Weisbecker]

struct task_struct::hardirq_threaded is set before the IRQ handler
is invoked but is not cleared back explicitly. This is done on the next
time trace_hardirq_enter() is called, which may be at best after softirq
execution, if any, or at worst by the next hardirq entry.

Besides being confusing, this exposes all the code in hardirq that
follows the handler outside lockdep vigilance concerning LD_WAIT_CONFIG
locking.

Lets rather be paranoid and make sure we properly define the end of an
LD_WAIT_CONFIG safe block.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/irqflags.h | 22 +++++++++++++++-------
 kernel/irq/handle.c      | 10 ++++++++--
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index a16adbb58f66..28481702460e 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -37,17 +37,24 @@
 # define trace_softirqs_enabled(p)	((p)->softirqs_enabled)
 # define trace_hardirq_enter()			\
 do {						\
-	if (!current->hardirq_context++)	\
-		current->hardirq_threaded = 0;	\
-} while (0)
-# define trace_hardirq_threaded()		\
-do {						\
-	current->hardirq_threaded = 1;		\
+	current->hardirq_context++;		\
 } while (0)
+
 # define trace_hardirq_exit()			\
 do {						\
 	current->hardirq_context--;		\
 } while (0)
+
+# define trace_hardirq_threaded()		\
+do {						\
+	current->hardirq_threaded = 1;		\
+} while (0)
+
+# define trace_hardirq_unthreaded()		\
+do {						\
+	current->hardirq_threaded = 0;		\
+} while (0)
+
 # define lockdep_softirq_enter()		\
 do {						\
 	current->softirq_context++;		\
@@ -98,8 +105,9 @@ do {						\
 # define trace_hardirqs_enabled(p)	0
 # define trace_softirqs_enabled(p)	0
 # define trace_hardirq_enter()		do { } while (0)
-# define trace_hardirq_threaded()	do { } while (0)
 # define trace_hardirq_exit()		do { } while (0)
+# define trace_hardirq_threaded()	do { } while (0)
+# define trace_hardirq_unthreaded()	do { } while (0)
 # define lockdep_softirq_enter()	do { } while (0)
 # define lockdep_softirq_exit()		do { } while (0)
 # define lockdep_hrtimer_enter(__hrtimer)		do { } while (0)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 16ee716e8291..39d6cf9f5853 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -144,18 +144,24 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
 
 	for_each_action_of_desc(desc, action) {
 		irqreturn_t res;
+		bool threadable;
 
 		/*
 		 * If this IRQ would be threaded under force_irqthreads, mark it so.
 		 */
-		if (irq_settings_can_thread(desc) &&
-		    !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)))
+		threadable = (irq_settings_can_thread(desc) &&
+			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
+
+		if (threadable)
 			trace_hardirq_threaded();
 
 		trace_irq_handler_entry(irq, action);
 		res = action->handler(irq, action->dev_id);
 		trace_irq_handler_exit(irq, action, res);
 
+		if (threadable)
+			trace_hardirq_unthreaded();
+
 		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pS enabled interrupts\n",
 			      irq, action->handler))
 			local_irq_disable();
-- 
2.25.0

[RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together

[Frederic Weisbecker]

These fields describe the same state: a code block running in hardirq
that might be threaded under specific configurations.

Merge them together in the same field. Also rename the result as
"hardirq_threadable" as we are talking about a possible state and not
an actual one.

While at it remove the posix cpu timer lockdep functions since we can
call into trace_hardirq_[un]threadable() directly. The need for hrtimer
binders is debatable as well.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/irqflags.h       | 70 ++++++++++++++--------------------
 include/linux/sched.h          |  3 +-
 kernel/irq/handle.c            |  4 +-
 kernel/locking/lockdep.c       |  2 +-
 kernel/time/posix-cpu-timers.c |  6 +--
 5 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 28481702460e..f69f93839760 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -45,14 +45,14 @@ do {						\
 	current->hardirq_context--;		\
 } while (0)
 
-# define trace_hardirq_threaded()		\
+# define trace_hardirq_threadable()		\
 do {						\
-	current->hardirq_threaded = 1;		\
+	current->hardirq_threadable = 1;	\
 } while (0)
 
-# define trace_hardirq_unthreaded()		\
+# define trace_hardirq_unthreadable()		\
 do {						\
-	current->hardirq_threaded = 0;		\
+	current->hardirq_threadable = 0;	\
 } while (0)
 
 # define lockdep_softirq_enter()		\
@@ -64,38 +64,16 @@ do {						\
 	current->softirq_context--;		\
 } while (0)
 
-# define lockdep_hrtimer_enter(__hrtimer)		\
-	  do {						\
-		  if (!__hrtimer->is_hard)		\
-			current->irq_config = 1;	\
-	  } while (0)
-
-# define lockdep_hrtimer_exit(__hrtimer)		\
-	  do {						\
-		  if (!__hrtimer->is_hard)		\
-			current->irq_config = 0;	\
-	  } while (0)
-
-# define lockdep_posixtimer_enter()				\
-	  do {							\
-		  current->irq_config = 1;			\
-	  } while (0)
-
-# define lockdep_posixtimer_exit()				\
-	  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;			\
-	  } while (0)
+do {									\
+	if (!(atomic_read(&__work->flags) & IRQ_WORK_HARD_IRQ))		\
+		trace_hardirq_threadable();				\
+} while (0)
 # define lockdep_irq_work_exit(__work)					\
-	  do {								\
-		  if (!(atomic_read(&__work->flags) & IRQ_WORK_HARD_IRQ))\
-			current->irq_config = 0;			\
-	  } while (0)
+do {									\
+	if (!(atomic_read(&__work->flags) & IRQ_WORK_HARD_IRQ))		\
+		trace_hardirq_unthreadable();				\
+} while (0)
 
 #else
 # define trace_hardirqs_on()		do { } while (0)
@@ -106,18 +84,26 @@ do {						\
 # define trace_softirqs_enabled(p)	0
 # define trace_hardirq_enter()		do { } while (0)
 # define trace_hardirq_exit()		do { } while (0)
-# define trace_hardirq_threaded()	do { } while (0)
-# define trace_hardirq_unthreaded()	do { } while (0)
+# define trace_hardirq_threadable()	do { } while (0)
+# define trace_hardirq_unthreadable()	do { } while (0)
 # define lockdep_softirq_enter()	do { } while (0)
 # 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_posixtimer_enter()		do { } while (0)
-# define lockdep_posixtimer_exit()		do { } while (0)
-# define lockdep_irq_work_enter(__work)		do { } while (0)
-# define lockdep_irq_work_exit(__work)		do { } while (0)
+# define lockdep_irq_work_enter(__work)	do { } while (0)
+# define lockdep_irq_work_exit(__work)	do { } while (0)
 #endif
 
+# define lockdep_hrtimer_enter(__hrtimer)	\
+do {						\
+	if (!__hrtimer->is_hard)		\
+		trace_hardirq_threadable();	\
+} while (0)
+
+# define lockdep_hrtimer_exit(__hrtimer)	\
+do {						\
+	if (!__hrtimer->is_hard)		\
+		trace_hardirq_unthreadable();	\
+} while (0)
+
 #if defined(CONFIG_IRQSOFF_TRACER) || \
 	defined(CONFIG_PREEMPT_TRACER)
  extern void stop_critical_timings(void);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 933914cdf219..9918d23b53e7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -970,7 +970,7 @@ struct task_struct {
 
 #ifdef CONFIG_TRACE_IRQFLAGS
 	unsigned int			irq_events;
-	unsigned int			hardirq_threaded;
+	unsigned int			hardirq_threadable;
 	unsigned long			hardirq_enable_ip;
 	unsigned long			hardirq_disable_ip;
 	unsigned int			hardirq_enable_event;
@@ -983,7 +983,6 @@ struct task_struct {
 	unsigned int			softirq_enable_event;
 	int				softirqs_enabled;
 	int				softirq_context;
-	int				irq_config;
 #endif
 
 #ifdef CONFIG_LOCKDEP
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 39d6cf9f5853..55405879b8ad 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -153,14 +153,14 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
 			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
 
 		if (threadable)
-			trace_hardirq_threaded();
+			trace_hardirq_threadable();
 
 		trace_irq_handler_entry(irq, action);
 		res = action->handler(irq, action->dev_id);
 		trace_irq_handler_exit(irq, action, res);
 
 		if (threadable)
-			trace_hardirq_unthreaded();
+			trace_hardirq_unthreadable();
 
 		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pS enabled interrupts\n",
 			      irq, action->handler))
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 0ebf9807d971..1cebaeb790a0 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4025,7 +4025,7 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
 		/*
 		 * Check if force_irqthreads will run us threaded.
 		 */
-		if (curr->hardirq_threaded || curr->irq_config)
+		if (curr->hardirq_threadable)
 			curr_inner = LD_WAIT_CONFIG;
 		else
 			curr_inner = LD_WAIT_SPIN;
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 2c48a7233b19..d29a06d60206 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1126,9 +1126,9 @@ void run_posix_cpu_timers(void)
 	if (!fastpath_timer_check(tsk))
 		return;
 
-	lockdep_posixtimer_enter();
+	trace_hardirq_threadable();
 	if (!lock_task_sighand(tsk, &flags)) {
-		lockdep_posixtimer_exit();
+		trace_hardirq_unthreadable();
 		return;
 	}
 	/*
@@ -1172,7 +1172,7 @@ void run_posix_cpu_timers(void)
 			cpu_timer_fire(timer);
 		spin_unlock(&timer->it_lock);
 	}
-	lockdep_posixtimer_exit();
+	trace_hardirq_unthreadable();
 }
 
 /*
-- 
2.25.0

[RFC PATCH 3/3] lockdep: Briefly comment current->hardirq_threadable usecases

[Frederic Weisbecker]

We have yet to find a proper comment for rcu_iw_handler() though.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/time/posix-cpu-timers.c | 1 +
 kernel/time/tick-sched.c       | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index d29a06d60206..85902d756e21 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1126,6 +1126,7 @@ void run_posix_cpu_timers(void)
 	if (!fastpath_timer_check(tsk))
 		return;
 
+	/* Should be offloaded to task_work at some future */
 	trace_hardirq_threadable();
 	if (!lock_task_sighand(tsk, &flags)) {
 		trace_hardirq_unthreadable();
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 3e2dc9b8858c..d469972673e4 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -245,6 +245,10 @@ 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,
+	/*
+	 * Must stay in hardirq. Threading would mess up with tick
+	 * dependencies.
+	 */
 	.flags = ATOMIC_INIT(IRQ_WORK_HARD_IRQ),
 };
 
-- 
2.25.0

Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end

[Peter Zijlstra]

On Mon, Mar 23, 2020 at 04:32:05AM +0100, Frederic Weisbecker wrote:
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -144,18 +144,24 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
>  
>  	for_each_action_of_desc(desc, action) {
>  		irqreturn_t res;
> +		bool threadable;
>  
>  		/*
>  		 * If this IRQ would be threaded under force_irqthreads, mark it so.
>  		 */
> -		if (irq_settings_can_thread(desc) &&
> -		    !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)))
> +		threadable = (irq_settings_can_thread(desc) &&
> +			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
> +
> +		if (threadable)
>  			trace_hardirq_threaded();
>  
>  		trace_irq_handler_entry(irq, action);
>  		res = action->handler(irq, action->dev_id);
>  		trace_irq_handler_exit(irq, action, res);
>  
> +		if (threadable)
> +			trace_hardirq_unthreaded();

AFAICT this doesn't work for nested IRQ handlers.

>  		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pS enabled interrupts\n",
>  			      irq, action->handler))
>  			local_irq_disable();
> -- 
> 2.25.0
> 

Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together

[Peter Zijlstra]

On Mon, Mar 23, 2020 at 04:32:06AM +0100, Frederic Weisbecker wrote:
> These fields describe the same state: a code block running in hardirq
> that might be threaded under specific configurations.
> 
> Merge them together in the same field. Also rename the result as
> "hardirq_threadable" as we are talking about a possible state and not
> an actual one.

What isn't instantly obvious is that they cannot overlap. For instance
mainline with force threaded interrupt handlers on, can't that have the
irq_work nest inside a threaded handler ?

I *think* it just about works out, but it definitely wants a little more
than this.

Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together

[Sebastian Andrzej Siewior]

On 2020-03-23 15:02:20 [+0100], Peter Zijlstra wrote:
> On Mon, Mar 23, 2020 at 04:32:06AM +0100, Frederic Weisbecker wrote:
> > These fields describe the same state: a code block running in hardirq
> > that might be threaded under specific configurations.
> > 
> > Merge them together in the same field. Also rename the result as
> > "hardirq_threadable" as we are talking about a possible state and not
> > an actual one.
> 
> What isn't instantly obvious is that they cannot overlap. For instance
> mainline with force threaded interrupt handlers on, can't that have the
> irq_work nest inside a threaded handler ?

I remember we kept them due to the nesting. A threaded-interrupt can be
interrupted by irq_work/hrtimer/posix-timer. 
So in a threaded interrupt it is okay to use a sleeping lock. It is not
okay with irq_work on-top - unless it is the non-atomic one.

> I *think* it just about works out, but it definitely wants a little more
> than this.

Sebastian

Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end

[Frederic Weisbecker]

On Mon, Mar 23, 2020 at 02:53:20PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 23, 2020 at 04:32:05AM +0100, Frederic Weisbecker wrote:
> > --- a/kernel/irq/handle.c
> > +++ b/kernel/irq/handle.c
> > @@ -144,18 +144,24 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
> >  
> >  	for_each_action_of_desc(desc, action) {
> >  		irqreturn_t res;
> > +		bool threadable;
> >  
> >  		/*
> >  		 * If this IRQ would be threaded under force_irqthreads, mark it so.
> >  		 */
> > -		if (irq_settings_can_thread(desc) &&
> > -		    !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)))
> > +		threadable = (irq_settings_can_thread(desc) &&
> > +			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
> > +
> > +		if (threadable)
> >  			trace_hardirq_threaded();
> >  
> >  		trace_irq_handler_entry(irq, action);
> >  		res = action->handler(irq, action->dev_id);
> >  		trace_irq_handler_exit(irq, action, res);
> >  
> > +		if (threadable)
> > +			trace_hardirq_unthreaded();
> 
> AFAICT this doesn't work for nested IRQ handlers.

So current->hardirq_threaded should be a counter perhaps?

> 
> >  		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pS enabled interrupts\n",
> >  			      irq, action->handler))
> >  			local_irq_disable();
> > -- 
> > 2.25.0
> > 

Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together

[Frederic Weisbecker]

On Mon, Mar 23, 2020 at 03:53:14PM +0100, Sebastian Andrzej Siewior wrote:
> On 2020-03-23 15:02:20 [+0100], Peter Zijlstra wrote:
> > On Mon, Mar 23, 2020 at 04:32:06AM +0100, Frederic Weisbecker wrote:
> > > These fields describe the same state: a code block running in hardirq
> > > that might be threaded under specific configurations.
> > > 
> > > Merge them together in the same field. Also rename the result as
> > > "hardirq_threadable" as we are talking about a possible state and not
> > > an actual one.
> > 
> > What isn't instantly obvious is that they cannot overlap. For instance
> > mainline with force threaded interrupt handlers on, can't that have the
> > irq_work nest inside a threaded handler ?
> 
> I remember we kept them due to the nesting. A threaded-interrupt can be
> interrupted by irq_work/hrtimer/posix-timer. 
> So in a threaded interrupt it is okay to use a sleeping lock. It is not
> okay with irq_work on-top - unless it is the non-atomic one.

Hmm, with the current layout which is:

    if (curr->hardirq_threaded || curr->irq_config)
        curr_inner = LD_WAIT_CONFIG;
    else
        curr_inner = LD_WAIT_SPIN;

you are not protected against that.

But anyway none of that should happen if hardirq_threaded and irq_config
are respectively only used when hardirq threads are disabled and irq_work
is always hardirq, right?

> 
> > I *think* it just about works out, but it definitely wants a little more
> > than this.
> 
> Sebastian

Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end

[Peter Zijlstra]

On Mon, Mar 23, 2020 at 04:30:40PM +0100, Frederic Weisbecker wrote:
> On Mon, Mar 23, 2020 at 02:53:20PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 23, 2020 at 04:32:05AM +0100, Frederic Weisbecker wrote:
> > > --- a/kernel/irq/handle.c
> > > +++ b/kernel/irq/handle.c
> > > @@ -144,18 +144,24 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
> > >  
> > >  	for_each_action_of_desc(desc, action) {
> > >  		irqreturn_t res;
> > > +		bool threadable;
> > >  
> > >  		/*
> > >  		 * If this IRQ would be threaded under force_irqthreads, mark it so.
> > >  		 */
> > > -		if (irq_settings_can_thread(desc) &&
> > > -		    !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)))
> > > +		threadable = (irq_settings_can_thread(desc) &&
> > > +			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
> > > +
> > > +		if (threadable)
> > >  			trace_hardirq_threaded();
> > >  
> > >  		trace_irq_handler_entry(irq, action);
> > >  		res = action->handler(irq, action->dev_id);
> > >  		trace_irq_handler_exit(irq, action, res);
> > >  
> > > +		if (threadable)
> > > +			trace_hardirq_unthreaded();
> > 
> > AFAICT this doesn't work for nested IRQ handlers.
> 
> So current->hardirq_threaded should be a counter perhaps?

Yeah, see how the old code used the hardirq_context counter for exactly
that. Also note how the old code was actually cheaper than this
(minimally, but still).

Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end

[Frederic Weisbecker]

On Mon, Mar 23, 2020 at 05:31:05PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 23, 2020 at 04:30:40PM +0100, Frederic Weisbecker wrote:
> > On Mon, Mar 23, 2020 at 02:53:20PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 23, 2020 at 04:32:05AM +0100, Frederic Weisbecker wrote:
> > > > --- a/kernel/irq/handle.c
> > > > +++ b/kernel/irq/handle.c
> > > > @@ -144,18 +144,24 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
> > > >  
> > > >  	for_each_action_of_desc(desc, action) {
> > > >  		irqreturn_t res;
> > > > +		bool threadable;
> > > >  
> > > >  		/*
> > > >  		 * If this IRQ would be threaded under force_irqthreads, mark it so.
> > > >  		 */
> > > > -		if (irq_settings_can_thread(desc) &&
> > > > -		    !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)))
> > > > +		threadable = (irq_settings_can_thread(desc) &&
> > > > +			      !(action->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT)));
> > > > +
> > > > +		if (threadable)
> > > >  			trace_hardirq_threaded();
> > > >  
> > > >  		trace_irq_handler_entry(irq, action);
> > > >  		res = action->handler(irq, action->dev_id);
> > > >  		trace_irq_handler_exit(irq, action, res);
> > > >  
> > > > +		if (threadable)
> > > > +			trace_hardirq_unthreaded();
> > > 
> > > AFAICT this doesn't work for nested IRQ handlers.
> > 
> > So current->hardirq_threaded should be a counter perhaps?
> 
> Yeah, see how the old code used the hardirq_context counter for exactly
> that. Also note how the old code was actually cheaper than this
> (minimally, but still).

But, how are nested hardirq handled currently? Isn't it with hardirq_context counter?