| Message ID | YgJ/XWVxxWDVBBVA@linutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [REPOST] irq_poll: Add local_bh_disable() in cpu_dead notifier | expand |
On Tue, Feb 08, 2022 at 03:34:05PM +0100, Sebastian Andrzej Siewior wrote: > __raise_softirq_irqoff() adds a bit to the pending sofirq mask and this > is it. The softirq won't be handled in a deterministic way but randomly > when an interrupt fires and handles the softirq in its irq_exit() routine or > if something randomly checks and handles pending softirqs in the call > chain before the CPU goes idle. > > Add a local_bh_disable/enable() around the IRQ-off section which will > handle pending softirqs. And I still haven't seen any good explanation of why this is useful.
On 2022-02-08 23:56:34 [-0800], Christoph Hellwig wrote: > On Tue, Feb 08, 2022 at 03:34:05PM +0100, Sebastian Andrzej Siewior wrote: > > __raise_softirq_irqoff() adds a bit to the pending sofirq mask and this > > is it. The softirq won't be handled in a deterministic way but randomly > > when an interrupt fires and handles the softirq in its irq_exit() routine or > > if something randomly checks and handles pending softirqs in the call > > chain before the CPU goes idle. > > > > Add a local_bh_disable/enable() around the IRQ-off section which will > > handle pending softirqs. > > And I still haven't seen any good explanation of why this is useful. You need to handle the pending softirqs. If you don't handle them immediately or in a deterministic say (like on IRQ exit) then they will be handled at a random point. If you don't handle them at all, the CPU will go idle and at least the NO_HZ will complain about pending softirqs (can_stop_idle_tick()). You could still argue that the CPU will go down and the there are latencies involved but… I want to avoid waking ksoftirqd for that since there is no need to wake it and the pending work can be done in-context, right away. Sebastian
On Thu, Feb 10, 2022 at 01:33:39PM +0100, Sebastian Andrzej Siewior wrote: > You need to handle the pending softirqs. If you don't handle them > immediately or in a deterministic say (like on IRQ exit) then they will > be handled at a random point. Yes. Just like regular interrupts.
On 2022-02-10 22:34:32 [-0800], Christoph Hellwig wrote: > On Thu, Feb 10, 2022 at 01:33:39PM +0100, Sebastian Andrzej Siewior wrote: > > You need to handle the pending softirqs. If you don't handle them > > immediately or in a deterministic say (like on IRQ exit) then they will > > be handled at a random point. > > Yes. Just like regular interrupts. With the exception that this one was already handled and should be handled and not delayed until the next interrupt. And as I said, on NO_HZ you get a warning about unhandled soft-irqs if the CPU goes idle. Sebastian
On Thu, Feb 10 2022 at 22:34, Christoph Hellwig wrote: > On Thu, Feb 10, 2022 at 01:33:39PM +0100, Sebastian Andrzej Siewior wrote: >> You need to handle the pending softirqs. If you don't handle them >> immediately or in a deterministic say (like on IRQ exit) then they will >> be handled at a random point. > > Yes. Just like regular interrupts. But interrupts make sure they are handled. This code does not and as Sebastian pointed out: "If you don't handle them at all, the CPU will go idle and at least the NO_HZ will complain about pending softirqs (can_stop_idle_tick())." That's clearly a bug, but this should be part of the changelog. Thanks, tglx
--- a/lib/irq_poll.c +++ b/lib/irq_poll.c @@ -191,11 +191,13 @@ static int irq_poll_cpu_dead(unsigned in * If a CPU goes away, splice its entries to the current CPU * and trigger a run of the softirq */ + local_bh_disable(); local_irq_disable(); list_splice_init(&per_cpu(blk_cpu_iopoll, cpu), this_cpu_ptr(&blk_cpu_iopoll)); __raise_softirq_irqoff(IRQ_POLL_SOFTIRQ); local_irq_enable(); + local_bh_enable(); return 0; }
__raise_softirq_irqoff() adds a bit to the pending sofirq mask and this is it. The softirq won't be handled in a deterministic way but randomly when an interrupt fires and handles the softirq in its irq_exit() routine or if something randomly checks and handles pending softirqs in the call chain before the CPU goes idle. Add a local_bh_disable/enable() around the IRQ-off section which will handle pending softirqs. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- Repost of https://lkml.kernel.org/r/20210930103754.2128949-1-bigeasy@linutronix.de lib/irq_poll.c | 2 ++ 1 file changed, 2 insertions(+)