From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:10
This is a follow up to the review comments of the series which makes
softirq processing PREEMPT_RT safe:
https://lore.kernel.org/r/20201207114743.GK3040@hirez.programming.kicks-ass.net
Peter suggested to replace the spin waiting in tasklet_disable() and
tasklet_kill() with wait_event(). This also gets rid of the ill defined
sched_yield() in tasklet_kill().
Analyzing all usage sites of tasklet_disable() and tasklet_unlock_wait() we
found that most of them are safe to be converted to a sleeping wait.
Only a few instances invoke tasklet_disable() from atomic context. A few
bugs which have been found in course of this analysis have been already
addressed seperately.
The following series takes the following approach:
1) Provide a variant of tasklet_disable() which can be invoked from
atomic contexts
2) Convert the usage sites which cannot be easily changed to a
sleepable wait to use this new function
3) Replace the spin waits in tasklet_disable() and tasklet_kill() with
sleepable variants.
If this is agreed on then the merging can be either done in bulk or the
first 4 patches could be applied on top of rc2 and tagged for consumption
in the relevant subsystem trees (networking, pci, firewire). In this case
the last patch which changes the implementation of tasklet_disable() has to
be post-poned until all other changes have reached mainline.
The series is also available from git:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git tasklet-2021-03-09
Thanks,
tglx
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:11
A barrier() in a tight loop which waits for something to happen on a remote
CPU is a pointless exercise. Replace it with cpu_relax() which allows HT
siblings to make progress.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/interrupt.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:12
Replacing the spin wait loops in tasklet_unlock_wait() with
wait_var_event() is not possible as a handful of tasklet_disable()
invocations are happening in atomic context. All other invocations are in
teardown paths which can sleep.
Provide tasklet_disable_in_atomic() and tasklet_unlock_spin_wait() to
convert the few atomic use cases over, which allows to change
tasklet_disable() and tasklet_unlock_wait() in a later step.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/interrupt.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:42
From: Peter Zijlstra <peterz@infradead.org>
tasklet_unlock_wait() spin waits for TASKLET_STATE_RUN to be cleared. This
is wasting CPU cycles in a tight loop which is especially painful in a
guest when the CPU running the tasklet is scheduled out.
tasklet_unlock_wait() is invoked from tasklet_kill() which is used in
teardown paths and not performance critical at all. Replace the spin wait
with wait_var_event().
There are no users of tasklet_unlock_wait() which are invoked from atomic
contexts. The usage in tasklet_disable() has been replaced temporarily with
the spin waiting variant until the atomic users are fixed up and will be
converted to the sleep wait variant later.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/interrupt.h | 13 ++-----------
kernel/softirq.c | 18 ++++++++++++++++++
2 files changed, 20 insertions(+), 11 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:43
From: Peter Zijlstra <peterz@infradead.org>
tasklet_kill() spin waits for TASKLET_STATE_SCHED to be cleared invoking
yield() from inside the loop. yield() is an ill defined mechanism and the
result might still be wasting CPU cycles in a tight loop which is
especially painful in a guest when the CPU running the tasklet is scheduled
out.
tasklet_kill() is used in teardown paths and not performance critical at
all. Replace the spin wait with wait_var_event().
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/softirq.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:44
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
tasklet_disable() is used in the timer callback. This might be distangled,
but without access to the hardware that's a bit risky.
Replace it with tasklet_disable_in_atomic() so tasklet_disable() can be
changed to a sleep wait once all remaining atomic users are converted.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: Denis Kirjanov <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
---
drivers/net/ethernet/dlink/sundance.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:44
tasklet_unlock_spin_wait() spin waits for the TASKLET_STATE_SCHED bit in
the tasklet state to be cleared. This works on !RT nicely because the
corresponding execution can only happen on a different CPU.
On RT softirq processing is preemptible, therefore a task preempting the
softirq processing thread can spin forever.
Prevent this by invoking local_bh_disable()/enable() inside the loop. In
case that the softirq processing thread was preempted by the current task,
current will block on the local lock which yields the CPU to the preempted
softirq processing thread. If the tasklet is processed on a different CPU
then the local_bh_disable()/enable() pair is just a waste of processor
cycles.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/interrupt.h | 2 +-
kernel/softirq.c | 28 +++++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:44
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
The link change tasklet disables the tasklets for tx/rx processing while
upating hw parameters and then enables the tasklets again.
This update can also be pushed into a workqueue where it can be performed
in preemptible context. This allows tasklet_disable() to become sleeping.
Replace the linkch_task tasklet with a work.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
---
drivers/net/ethernet/jme.c | 10 +++++-----
drivers/net/ethernet/jme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:45
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
The atmdev_ops::send callback which calls tasklet_disable() is invoked with
bottom halfs disabled from net_device_ops::ndo_start_xmit(). All other
invocations of tasklet_disable() in this driver happen in preemptible
context.
Change the send() call to use tasklet_disable_in_atomic() which allows
tasklet_disable() to be made sleepable once the remaining atomic context
usage sites are cleaned up.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: Chas Williams <3chas3@gmail.com>
Cc: linux-atm-general@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
drivers/atm/eni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/atm/eni.c+++ b/drivers/atm/eni.c
@@ -2054,7 +2054,7 @@ static int eni_send(struct atm_vcc *vcc,}submitted++;ATM_SKB(skb)->vcc=vcc;-tasklet_disable(&ENI_DEV(vcc->dev)->task);+tasklet_disable_in_atomic(&ENI_DEV(vcc->dev)->task);res=do_tx(skb);tasklet_enable(&ENI_DEV(vcc->dev)->task);if(res==enq_ok)return0;
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:45
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
tasklet_disable() is invoked in several places. Some of them are in atomic
context which prevents a conversion of tasklet_disable() to a sleepable
function.
The atomic callchains are:
ar_context_tasklet()
ohci_cancel_packet()
tasklet_disable()
...
ohci_flush_iso_completions()
tasklet_disable()
The invocation of tasklet_disable() from at_context_flush() is always in
preemptible context.
Use tasklet_disable_in_atomic() for the two invocations in
ohci_cancel_packet() and ohci_flush_iso_completions().
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: linux1394-devel@lists.sourceforge.net
---
drivers/firewire/ohci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:45
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
The hv_compose_msi_msg() callback in irq_chip::irq_compose_msi_msg is
invoked via irq_chip_compose_msi_msg(), which itself is always invoked from
atomic contexts from the guts of the interrupt core code.
There is no way to change this w/o rewriting the whole driver, so use
tasklet_disable_in_atomic() which allows to make tasklet_disable()
sleepable once the remaining atomic users are addressed.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-hyperv@vger.kernel.org
Cc: linux-pci@vger.kernel.org
---
drivers/pci/controller/pci-hyperv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Tue, Mar 09, 2021 at 09:42:15AM +0100, Thomas Gleixner wrote:
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
The hv_compose_msi_msg() callback in irq_chip::irq_compose_msi_msg is
invoked via irq_chip_compose_msi_msg(), which itself is always invoked from
atomic contexts from the guts of the interrupt core code.
There is no way to change this w/o rewriting the whole driver, so use
tasklet_disable_in_atomic() which allows to make tasklet_disable()
sleepable once the remaining atomic users are addressed.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-hyperv@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
It'd be ideal if you could merge this as a group. Let me know if you
want me to do anything else.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-03-10 11:35:29
On Tue, Mar 09, 2021 at 09:42:15AM +0100, Thomas Gleixner wrote:
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
The hv_compose_msi_msg() callback in irq_chip::irq_compose_msi_msg is
invoked via irq_chip_compose_msi_msg(), which itself is always invoked from
atomic contexts from the guts of the interrupt core code.
There is no way to change this w/o rewriting the whole driver, so use
tasklet_disable_in_atomic() which allows to make tasklet_disable()
sleepable once the remaining atomic users are addressed.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:45
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
All callers of ath9k_beacon_ensure_primary_slot() are preemptible /
acquire a mutex except for this callchain:
spin_lock_bh(&sc->sc_pcu_lock);
ath_complete_reset()
-> ath9k_calculate_summary_state()
-> ath9k_beacon_ensure_primary_slot()
It's unclear how that can be distangled, so use tasklet_disable_in_atomic()
for now. This allows tasklet_disable() to become sleepable once the
remaining atomic users are cleaned up.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <redacted>
Cc: ath9k-devel@qca.qualcomm.com
Cc: Kalle Valo <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
drivers/net/wireless/ath/ath9k/beacon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:46:46
-- NOT FOR IMMEDIATE MERGING --
Now that all users of tasklet_disable() are invoked from sleepable context,
convert it to use tasklet_unlock_wait() which might sleep.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/interrupt.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2021-03-09 08:47:12
To ease the transition use spin waiting in tasklet_disable() until all
usage sites from atomic context have been cleaned up.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/interrupt.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-09 14:06:13
On Tue, Mar 09, 2021 at 09:42:03AM +0100, Thomas Gleixner wrote:
This is a follow up to the review comments of the series which makes
softirq processing PREEMPT_RT safe:
https://lore.kernel.org/r/20201207114743.GK3040@hirez.programming.kicks-ass.net
Peter suggested to replace the spin waiting in tasklet_disable() and
tasklet_kill() with wait_event(). This also gets rid of the ill defined
sched_yield() in tasklet_kill().
Analyzing all usage sites of tasklet_disable() and tasklet_unlock_wait() we
found that most of them are safe to be converted to a sleeping wait.
Only a few instances invoke tasklet_disable() from atomic context. A few
bugs which have been found in course of this analysis have been already
addressed seperately.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>