* [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end
2020-03-23 3:32 [RFC PATCH 0/3] lockdep/irq: wait-type related cleanups Frederic Weisbecker
@ 2020-03-23 3:32 ` Frederic Weisbecker
2020-03-23 13:53 ` Peter Zijlstra
2020-03-23 3:32 ` [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together Frederic Weisbecker
2020-03-23 3:32 ` [RFC PATCH 3/3] lockdep: Briefly comment current->hardirq_threadable usecases Frederic Weisbecker
2 siblings, 1 reply; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-23 3:32 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Sebastian Andrzej Siewior, Peter Zijlstra,
Paul E . McKenney, Thomas Gleixner
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end
2020-03-23 3:32 ` [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end Frederic Weisbecker
@ 2020-03-23 13:53 ` Peter Zijlstra
2020-03-23 15:30 ` Frederic Weisbecker
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-03-23 13:53 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Sebastian Andrzej Siewior, Paul E . McKenney,
Thomas Gleixner
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end
2020-03-23 13:53 ` Peter Zijlstra
@ 2020-03-23 15:30 ` Frederic Weisbecker
2020-03-23 16:31 ` Peter Zijlstra
0 siblings, 1 reply; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-23 15:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: LKML, Sebastian Andrzej Siewior, Paul E . McKenney,
Thomas Gleixner
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
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end
2020-03-23 15:30 ` Frederic Weisbecker
@ 2020-03-23 16:31 ` Peter Zijlstra
2020-03-24 3:20 ` Frederic Weisbecker
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-03-23 16:31 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Sebastian Andrzej Siewior, Paul E . McKenney,
Thomas Gleixner
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).
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end
2020-03-23 16:31 ` Peter Zijlstra
@ 2020-03-24 3:20 ` Frederic Weisbecker
0 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-24 3:20 UTC (permalink / raw)
To: Peter Zijlstra
Cc: LKML, Sebastian Andrzej Siewior, Paul E . McKenney,
Thomas Gleixner
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together
2020-03-23 3:32 [RFC PATCH 0/3] lockdep/irq: wait-type related cleanups Frederic Weisbecker
2020-03-23 3:32 ` [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end Frederic Weisbecker
@ 2020-03-23 3:32 ` Frederic Weisbecker
2020-03-23 14:02 ` Peter Zijlstra
2020-03-23 3:32 ` [RFC PATCH 3/3] lockdep: Briefly comment current->hardirq_threadable usecases Frederic Weisbecker
2 siblings, 1 reply; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-23 3:32 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Sebastian Andrzej Siewior, Peter Zijlstra,
Paul E . McKenney, Thomas Gleixner
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together
2020-03-23 3:32 ` [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together Frederic Weisbecker
@ 2020-03-23 14:02 ` Peter Zijlstra
2020-03-23 14:53 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-03-23 14:02 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: LKML, Sebastian Andrzej Siewior, Paul E . McKenney,
Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together
2020-03-23 14:02 ` Peter Zijlstra
@ 2020-03-23 14:53 ` Sebastian Andrzej Siewior
2020-03-23 16:14 ` Frederic Weisbecker
0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-03-23 14:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Frederic Weisbecker, LKML, Paul E . McKenney, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together
2020-03-23 14:53 ` Sebastian Andrzej Siewior
@ 2020-03-23 16:14 ` Frederic Weisbecker
0 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-23 16:14 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Peter Zijlstra, LKML, Paul E . McKenney, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 3/3] lockdep: Briefly comment current->hardirq_threadable usecases
2020-03-23 3:32 [RFC PATCH 0/3] lockdep/irq: wait-type related cleanups Frederic Weisbecker
2020-03-23 3:32 ` [RFC PATCH 1/3] lockdep/irq: Be more strict about IRQ-threadable code end Frederic Weisbecker
2020-03-23 3:32 ` [RFC PATCH 2/3] lockdep: Merge hardirq_threaded and irq_config together Frederic Weisbecker
@ 2020-03-23 3:32 ` Frederic Weisbecker
2 siblings, 0 replies; 11+ messages in thread
From: Frederic Weisbecker @ 2020-03-23 3:32 UTC (permalink / raw)
To: LKML
Cc: Frederic Weisbecker, Sebastian Andrzej Siewior, Peter Zijlstra,
Paul E . McKenney, Thomas Gleixner
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
^ permalink raw reply related [flat|nested] 11+ messages in thread