linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head
@ 2021-01-23 20:10 Sebastian Andrzej Siewior
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
                   ` (4 more replies)
  0 siblings, 5 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-23 20:10 UTC (permalink / raw)
  To: linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar

Patch 2+3 were applied and then dropped by Jens due to a NOHZ+softirq
related warning [0]. Turns out a successful wakeup via
set_nr_if_polling() will not process any softirqs and the CPU may go
back to idle. This is addressed by patch #1.

smpcfd_dying_cpu() will also invoke SMP-functions calls via
flush_smp_call_function_queue() but the block layer shouldn't queue
anything because the CPU isn't online anymore.
The two caller of flush_smp_call_function_from_idle() look fine with
opening interrupts from within do_softirq().

[0] https://lkml.kernel.org/r/1ee4b31b-350e-a9f5-4349-cfb34b89829a@kernel.dk

Sebastian



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
@ 2021-01-23 20:10 ` Sebastian Andrzej Siewior
  2021-02-01 19:35   ` Sebastian Andrzej Siewior
                     ` (3 more replies)
  2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
                   ` (3 subsequent siblings)
  4 siblings, 4 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-23 20:10 UTC (permalink / raw)
  To: linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Sebastian Andrzej Siewior

send_call_function_single_ipi() may wake an idle CPU without sending an
IPI. The woken up CPU will process the SMP-functions in
flush_smp_call_function_from_idle(). Any raised softirq from within the
SMP-function call will not be processed.
Should the CPU have no tasks assigned, then it will go back to idle with
pending softirqs and the NOHZ will rightfully complain.

Process pending softirqs on return from flush_smp_call_function_queue().

Fixes: b2a02fc43a1f4 ("smp: Optimize send_call_function_single_ipi()")
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/smp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/smp.c b/kernel/smp.c
index 1b6070bf97bb0..aeb0adfa06063 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -14,6 +14,7 @@
 #include <linux/export.h>
 #include <linux/percpu.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
@@ -449,6 +450,9 @@ void flush_smp_call_function_from_idle(void)
 
 	local_irq_save(flags);
 	flush_smp_call_function_queue(true);
+	if (local_softirq_pending())
+		do_softirq();
+
 	local_irq_restore(flags);
 }
 
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
@ 2021-01-23 20:10 ` Sebastian Andrzej Siewior
  2021-01-25  7:10   ` Hannes Reinecke
                     ` (2 more replies)
  2021-01-23 20:10 ` [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done Sebastian Andrzej Siewior
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-23 20:10 UTC (permalink / raw)
  To: linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Sebastian Andrzej Siewior

Controllers with multiple queues have their IRQ-handelers pinned to a
CPU. The core shouldn't need to complete the request on a remote CPU.

Remove this case and always raise the softirq to complete the request.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 block/blk-mq.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f285a9123a8b0..90348ae518461 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -628,19 +628,7 @@ static void __blk_mq_complete_request_remote(void *data)
 {
 	struct request *rq = data;
 
-	/*
-	 * For most of single queue controllers, there is only one irq vector
-	 * for handling I/O completion, and the only irq's affinity is set
-	 * to all possible CPUs.  On most of ARCHs, this affinity means the irq
-	 * is handled on one specific CPU.
-	 *
-	 * So complete I/O requests in softirq context in case of single queue
-	 * devices to avoid degrading I/O performance due to irqsoff latency.
-	 */
-	if (rq->q->nr_hw_queues == 1)
-		blk_mq_trigger_softirq(rq);
-	else
-		rq->q->mq_ops->complete(rq);
+	blk_mq_trigger_softirq(rq);
 }
 
 static inline bool blk_mq_complete_need_ipi(struct request *rq)
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done
  2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
  2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
@ 2021-01-23 20:10 ` Sebastian Andrzej Siewior
  2021-01-25  8:30   ` Christoph Hellwig
  2021-01-25  4:27 ` [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Jens Axboe
  2021-02-10 14:43 ` Jens Axboe
  4 siblings, 1 reply; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-23 20:10 UTC (permalink / raw)
  To: linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
	Sebastian Andrzej Siewior

With llist_head it is possible to avoid the locking (the irq-off region)
when items are added. This makes it possible to add items on a remote
CPU without additional locking.
llist_add() returns true if the list was previously empty. This can be
used to invoke the SMP function call / raise sofirq only if the first
item was added (otherwise it is already pending).
This simplifies the code a little and reduces the IRQ-off regions.

blk_mq_raise_softirq() needs a preempt-disable section to ensure the
request is enqueued on the same CPU as the softirq is raised.
Some callers (USB-storage) invoke this path in preemptible context.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 block/blk-mq.c         | 97 ++++++++++++++++++------------------------
 include/linux/blkdev.h |  2 +-
 2 files changed, 42 insertions(+), 57 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 90348ae518461..463de2981df8a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -41,7 +41,7 @@
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
 
-static DEFINE_PER_CPU(struct list_head, blk_cpu_done);
+static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 
 static void blk_mq_poll_stats_start(struct request_queue *q);
 static void blk_mq_poll_stats_fn(struct blk_stat_callback *cb);
@@ -567,68 +567,29 @@ void blk_mq_end_request(struct request *rq, blk_status_t error)
 }
 EXPORT_SYMBOL(blk_mq_end_request);
 
-/*
- * Softirq action handler - move entries to local list and loop over them
- * while passing them to the queue registered handler.
- */
-static __latent_entropy void blk_done_softirq(struct softirq_action *h)
+static void blk_complete_reqs(struct llist_head *list)
 {
-	struct list_head *cpu_list, local_list;
+	struct llist_node *entry = llist_reverse_order(llist_del_all(list));
+	struct request *rq, *next;
 
-	local_irq_disable();
-	cpu_list = this_cpu_ptr(&blk_cpu_done);
-	list_replace_init(cpu_list, &local_list);
-	local_irq_enable();
-
-	while (!list_empty(&local_list)) {
-		struct request *rq;
-
-		rq = list_entry(local_list.next, struct request, ipi_list);
-		list_del_init(&rq->ipi_list);
+	llist_for_each_entry_safe(rq, next, entry, ipi_list)
 		rq->q->mq_ops->complete(rq);
-	}
 }
 
-static void blk_mq_trigger_softirq(struct request *rq)
+static __latent_entropy void blk_done_softirq(struct softirq_action *h)
 {
-	struct list_head *list;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	list = this_cpu_ptr(&blk_cpu_done);
-	list_add_tail(&rq->ipi_list, list);
-
-	/*
-	 * If the list only contains our just added request, signal a raise of
-	 * the softirq.  If there are already entries there, someone already
-	 * raised the irq but it hasn't run yet.
-	 */
-	if (list->next == &rq->ipi_list)
-		raise_softirq_irqoff(BLOCK_SOFTIRQ);
-	local_irq_restore(flags);
+	blk_complete_reqs(this_cpu_ptr(&blk_cpu_done));
 }
 
 static int blk_softirq_cpu_dead(unsigned int cpu)
 {
-	/*
-	 * If a CPU goes away, splice its entries to the current CPU
-	 * and trigger a run of the softirq
-	 */
-	local_irq_disable();
-	list_splice_init(&per_cpu(blk_cpu_done, cpu),
-			 this_cpu_ptr(&blk_cpu_done));
-	raise_softirq_irqoff(BLOCK_SOFTIRQ);
-	local_irq_enable();
-
+	blk_complete_reqs(&per_cpu(blk_cpu_done, cpu));
 	return 0;
 }
 
-
 static void __blk_mq_complete_request_remote(void *data)
 {
-	struct request *rq = data;
-
-	blk_mq_trigger_softirq(rq);
+	__raise_softirq_irqoff(BLOCK_SOFTIRQ);
 }
 
 static inline bool blk_mq_complete_need_ipi(struct request *rq)
@@ -657,6 +618,30 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq)
 	return cpu_online(rq->mq_ctx->cpu);
 }
 
+static void blk_mq_complete_send_ipi(struct request *rq)
+{
+	struct llist_head *list;
+	unsigned int cpu;
+
+	cpu = rq->mq_ctx->cpu;
+	list = &per_cpu(blk_cpu_done, cpu);
+	if (llist_add(&rq->ipi_list, list)) {
+		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
+		smp_call_function_single_async(cpu, &rq->csd);
+	}
+}
+
+static void blk_mq_raise_softirq(struct request *rq)
+{
+	struct llist_head *list;
+
+	preempt_disable();
+	list = this_cpu_ptr(&blk_cpu_done);
+	if (llist_add(&rq->ipi_list, list))
+		raise_softirq(BLOCK_SOFTIRQ);
+	preempt_enable();
+}
+
 bool blk_mq_complete_request_remote(struct request *rq)
 {
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
@@ -669,15 +654,15 @@ bool blk_mq_complete_request_remote(struct request *rq)
 		return false;
 
 	if (blk_mq_complete_need_ipi(rq)) {
-		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
-		smp_call_function_single_async(rq->mq_ctx->cpu, &rq->csd);
-	} else {
-		if (rq->q->nr_hw_queues > 1)
-			return false;
-		blk_mq_trigger_softirq(rq);
+		blk_mq_complete_send_ipi(rq);
+		return true;
 	}
 
-	return true;
+	if (rq->q->nr_hw_queues == 1) {
+		blk_mq_raise_softirq(rq);
+		return true;
+	}
+	return false;
 }
 EXPORT_SYMBOL_GPL(blk_mq_complete_request_remote);
 
@@ -3892,7 +3877,7 @@ static int __init blk_mq_init(void)
 	int i;
 
 	for_each_possible_cpu(i)
-		INIT_LIST_HEAD(&per_cpu(blk_cpu_done, i));
+		init_llist_head(&per_cpu(blk_cpu_done, i));
 	open_softirq(BLOCK_SOFTIRQ, blk_done_softirq);
 
 	cpuhp_setup_state_nocalls(CPUHP_BLOCK_SOFTIRQ_DEAD,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f94ee3089e015..89a444c5a5833 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -153,7 +153,7 @@ struct request {
 	 */
 	union {
 		struct hlist_node hash;	/* merge hash */
-		struct list_head ipi_list;
+		struct llist_node ipi_list;
 	};
 
 	/*
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head
  2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
                   ` (2 preceding siblings ...)
  2021-01-23 20:10 ` [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done Sebastian Andrzej Siewior
@ 2021-01-25  4:27 ` Jens Axboe
  2021-02-10 14:43 ` Jens Axboe
  4 siblings, 0 replies; 25+ messages in thread
From: Jens Axboe @ 2021-01-25  4:27 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-block, linux-kernel
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 1/23/21 1:10 PM, Sebastian Andrzej Siewior wrote:
> Patch 2+3 were applied and then dropped by Jens due to a NOHZ+softirq
> related warning [0]. Turns out a successful wakeup via
> set_nr_if_polling() will not process any softirqs and the CPU may go
> back to idle. This is addressed by patch #1.
> 
> smpcfd_dying_cpu() will also invoke SMP-functions calls via
> flush_smp_call_function_queue() but the block layer shouldn't queue
> anything because the CPU isn't online anymore.
> The two caller of flush_smp_call_function_from_idle() look fine with
> opening interrupts from within do_softirq().
> 
> [0] https://lkml.kernel.org/r/1ee4b31b-350e-a9f5-4349-cfb34b89829a@kernel.dk

I can queue up the block side once the IPI fix is in some stable branch
that I can pull in.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
@ 2021-01-25  7:10   ` Hannes Reinecke
  2021-01-25  8:25     ` Christoph Hellwig
  2021-01-25  8:22   ` Christoph Hellwig
  2021-01-27 11:22   ` Daniel Wagner
  2 siblings, 1 reply; 25+ messages in thread
From: Hannes Reinecke @ 2021-01-25  7:10 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 1/23/21 9:10 PM, Sebastian Andrzej Siewior wrote:
> Controllers with multiple queues have their IRQ-handelers pinned to a
> CPU. The core shouldn't need to complete the request on a remote CPU.
> 
> Remove this case and always raise the softirq to complete the request.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   block/blk-mq.c | 14 +-------------
>   1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index f285a9123a8b0..90348ae518461 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -628,19 +628,7 @@ static void __blk_mq_complete_request_remote(void *data)
>   {
>   	struct request *rq = data;
>   
> -	/*
> -	 * For most of single queue controllers, there is only one irq vector
> -	 * for handling I/O completion, and the only irq's affinity is set
> -	 * to all possible CPUs.  On most of ARCHs, this affinity means the irq
> -	 * is handled on one specific CPU.
> -	 *
> -	 * So complete I/O requests in softirq context in case of single queue
> -	 * devices to avoid degrading I/O performance due to irqsoff latency.
> -	 */
> -	if (rq->q->nr_hw_queues == 1)
> -		blk_mq_trigger_softirq(rq);
> -	else
> -		rq->q->mq_ops->complete(rq);
> +	blk_mq_trigger_softirq(rq);
>   }
>   
>   static inline bool blk_mq_complete_need_ipi(struct request *rq)
> 
I don't get this.
This code is about _avoiding_ having to raise a softirq if the driver 
exports more than one hardware queue.
So where exactly does the remote CPU case come in here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
  2021-01-25  7:10   ` Hannes Reinecke
@ 2021-01-25  8:22   ` Christoph Hellwig
  2021-01-25  8:49     ` Christoph Hellwig
  2021-01-27 11:22   ` Daniel Wagner
  2 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:22 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

On Sat, Jan 23, 2021 at 09:10:26PM +0100, Sebastian Andrzej Siewior wrote:
> Controllers with multiple queues have their IRQ-handelers pinned to a
> CPU. The core shouldn't need to complete the request on a remote CPU.
> 
> Remove this case and always raise the softirq to complete the request.

What about changing blk_mq_trigger_softirq to take a void * argument
and thus removing __blk_mq_complete_request_remote entirely?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-25  7:10   ` Hannes Reinecke
@ 2021-01-25  8:25     ` Christoph Hellwig
  2021-01-25  8:30       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:25 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Sebastian Andrzej Siewior, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On Mon, Jan 25, 2021 at 08:10:16AM +0100, Hannes Reinecke wrote:
> I don't get this.
> This code is about _avoiding_ having to raise a softirq if the driver
> exports more than one hardware queue.
> So where exactly does the remote CPU case come in here?

__blk_mq_complete_request_remote is only called for the case where we
do not completelky locally.  The case that "degrades" here is where
the device supports multiple queues, but less than the number of CPUs,
and we bounce the completion to another CPU.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done
  2021-01-23 20:10 ` [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done Sebastian Andrzej Siewior
@ 2021-01-25  8:30   ` Christoph Hellwig
  2021-01-25  8:32     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:30 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

> +static void blk_mq_complete_send_ipi(struct request *rq)
> +{
> +	struct llist_head *list;
> +	unsigned int cpu;
> +
> +	cpu = rq->mq_ctx->cpu;
> +	list = &per_cpu(blk_cpu_done, cpu);
> +	if (llist_add(&rq->ipi_list, list)) {
> +		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
> +		smp_call_function_single_async(cpu, &rq->csd);
> +	}
> +}

Nit: it would be nice to initialize cpu and list in the declaration
lines.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-25  8:25     ` Christoph Hellwig
@ 2021-01-25  8:30       ` Sebastian Andrzej Siewior
  2021-01-25  8:32         ` Christoph Hellwig
  0 siblings, 1 reply; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-25  8:30 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Hannes Reinecke, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 2021-01-25 08:25:42 [+0000], Christoph Hellwig wrote:
> On Mon, Jan 25, 2021 at 08:10:16AM +0100, Hannes Reinecke wrote:
> > I don't get this.
> > This code is about _avoiding_ having to raise a softirq if the driver
> > exports more than one hardware queue.
> > So where exactly does the remote CPU case come in here?
> 
> __blk_mq_complete_request_remote is only called for the case where we
> do not completelky locally.  The case that "degrades" here is where
> the device supports multiple queues, but less than the number of CPUs,
> and we bounce the completion to another CPU.

Does it really "degrade" or just use the softirq more often? The usual
case is run the softirqs in irq_exit() which is just after IPI.

Sebastian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done
  2021-01-25  8:30   ` Christoph Hellwig
@ 2021-01-25  8:32     ` Sebastian Andrzej Siewior
  2021-01-25  8:39       ` Christoph Hellwig
  0 siblings, 1 reply; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-25  8:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

On 2021-01-25 08:30:12 [+0000], Christoph Hellwig wrote:
> > +static void blk_mq_complete_send_ipi(struct request *rq)
> > +{
> > +	struct llist_head *list;
> > +	unsigned int cpu;
> > +
> > +	cpu = rq->mq_ctx->cpu;
> > +	list = &per_cpu(blk_cpu_done, cpu);
> > +	if (llist_add(&rq->ipi_list, list)) {
> > +		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
> > +		smp_call_function_single_async(cpu, &rq->csd);
> > +	}
> > +}
> 
> Nit: it would be nice to initialize cpu and list in the declaration
> lines.

Why? They get initialized later.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Sebastian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-25  8:30       ` Sebastian Andrzej Siewior
@ 2021-01-25  8:32         ` Christoph Hellwig
  2021-01-25  9:29           ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:32 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Christoph Hellwig, Hannes Reinecke, linux-block, linux-kernel,
	Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On Mon, Jan 25, 2021 at 09:30:29AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-01-25 08:25:42 [+0000], Christoph Hellwig wrote:
> > On Mon, Jan 25, 2021 at 08:10:16AM +0100, Hannes Reinecke wrote:
> > > I don't get this.
> > > This code is about _avoiding_ having to raise a softirq if the driver
> > > exports more than one hardware queue.
> > > So where exactly does the remote CPU case come in here?
> > 
> > __blk_mq_complete_request_remote is only called for the case where we
> > do not completelky locally.  The case that "degrades" here is where
> > the device supports multiple queues, but less than the number of CPUs,
> > and we bounce the completion to another CPU.
> 
> Does it really "degrade" or just use the softirq more often? The usual
> case is run the softirqs in irq_exit() which is just after IPI.

Well, I put it in quotes because I'm not sure what the exact effect
is.  But we do delay these completions to the softirq now instead of
hardirq context, which at least in theory increases latency.  OTOH it
might even have positive effects on the rest of the system.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done
  2021-01-25  8:32     ` Sebastian Andrzej Siewior
@ 2021-01-25  8:39       ` Christoph Hellwig
  2021-01-25  9:54         ` [PATCH 3/3 v2] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:39 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Christoph Hellwig, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On Mon, Jan 25, 2021 at 09:32:04AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-01-25 08:30:12 [+0000], Christoph Hellwig wrote:
> > > +static void blk_mq_complete_send_ipi(struct request *rq)
> > > +{
> > > +	struct llist_head *list;
> > > +	unsigned int cpu;
> > > +
> > > +	cpu = rq->mq_ctx->cpu;
> > > +	list = &per_cpu(blk_cpu_done, cpu);
> > > +	if (llist_add(&rq->ipi_list, list)) {
> > > +		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
> > > +		smp_call_function_single_async(cpu, &rq->csd);
> > > +	}
> > > +}
> > 
> > Nit: it would be nice to initialize cpu and list in the declaration
> > lines.
> 
> Why? They get initialized later.

Because:

	unsigned int cpu = rq->mq_ctx->cpu;
	struct llist_head *list = &per_cpu(blk_cpu_done, cpu);

is a lot easier to follow than:

	struct llist_head *list;
	unsigned int cpu;

	cpu = rq->mq_ctx->cpu;
	list = &per_cpu(blk_cpu_done, cpu);


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-25  8:22   ` Christoph Hellwig
@ 2021-01-25  8:49     ` Christoph Hellwig
  0 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25  8:49 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

On Mon, Jan 25, 2021 at 08:23:03AM +0000, Christoph Hellwig wrote:
> On Sat, Jan 23, 2021 at 09:10:26PM +0100, Sebastian Andrzej Siewior wrote:
> > Controllers with multiple queues have their IRQ-handelers pinned to a
> > CPU. The core shouldn't need to complete the request on a remote CPU.
> > 
> > Remove this case and always raise the softirq to complete the request.
> 
> What about changing blk_mq_trigger_softirq to take a void * argument
> and thus removing __blk_mq_complete_request_remote entirely?

I'll take this back - that change is in the way of what you do in patch
3.  So this looks good as-is:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-25  8:32         ` Christoph Hellwig
@ 2021-01-25  9:29           ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-25  9:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Hannes Reinecke, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 2021-01-25 08:32:48 [+0000], Christoph Hellwig wrote:
> Well, I put it in quotes because I'm not sure what the exact effect
> is.  But we do delay these completions to the softirq now instead of
> hardirq context, which at least in theory increases latency.  OTOH it
> might even have positive effects on the rest of the system.

The last part is/was my motivation ;)

Sebastian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 3/3 v2] blk-mq: Use llist_head for blk_cpu_done
  2021-01-25  8:39       ` Christoph Hellwig
@ 2021-01-25  9:54         ` Sebastian Andrzej Siewior
  2021-01-25 10:14           ` Christoph Hellwig
  2021-01-27 11:23           ` Daniel Wagner
  0 siblings, 2 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-01-25  9:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

With llist_head it is possible to avoid the locking (the irq-off region)
when items are added. This makes it possible to add items on a remote
CPU without additional locking.
llist_add() returns true if the list was previously empty. This can be
used to invoke the SMP function call / raise sofirq only if the first
item was added (otherwise it is already pending).
This simplifies the code a little and reduces the IRQ-off regions.

blk_mq_raise_softirq() needs a preempt-disable section to ensure the
request is enqueued on the same CPU as the softirq is raised.
Some callers (USB-storage) invoke this path in preemptible context.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Move var initialisation to declaration in
       blk_mq_complete_send_ipi(). Suggested by hch.

 block/blk-mq.c         | 95 +++++++++++++++++-------------------------
 include/linux/blkdev.h |  2 +-
 2 files changed, 40 insertions(+), 57 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 90348ae518461..8429be0d9b8dd 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -41,7 +41,7 @@
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
 
-static DEFINE_PER_CPU(struct list_head, blk_cpu_done);
+static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 
 static void blk_mq_poll_stats_start(struct request_queue *q);
 static void blk_mq_poll_stats_fn(struct blk_stat_callback *cb);
@@ -567,68 +567,29 @@ void blk_mq_end_request(struct request *rq, blk_status_t error)
 }
 EXPORT_SYMBOL(blk_mq_end_request);
 
-/*
- * Softirq action handler - move entries to local list and loop over them
- * while passing them to the queue registered handler.
- */
-static __latent_entropy void blk_done_softirq(struct softirq_action *h)
+static void blk_complete_reqs(struct llist_head *list)
 {
-	struct list_head *cpu_list, local_list;
+	struct llist_node *entry = llist_reverse_order(llist_del_all(list));
+	struct request *rq, *next;
 
-	local_irq_disable();
-	cpu_list = this_cpu_ptr(&blk_cpu_done);
-	list_replace_init(cpu_list, &local_list);
-	local_irq_enable();
-
-	while (!list_empty(&local_list)) {
-		struct request *rq;
-
-		rq = list_entry(local_list.next, struct request, ipi_list);
-		list_del_init(&rq->ipi_list);
+	llist_for_each_entry_safe(rq, next, entry, ipi_list)
 		rq->q->mq_ops->complete(rq);
-	}
 }
 
-static void blk_mq_trigger_softirq(struct request *rq)
+static __latent_entropy void blk_done_softirq(struct softirq_action *h)
 {
-	struct list_head *list;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	list = this_cpu_ptr(&blk_cpu_done);
-	list_add_tail(&rq->ipi_list, list);
-
-	/*
-	 * If the list only contains our just added request, signal a raise of
-	 * the softirq.  If there are already entries there, someone already
-	 * raised the irq but it hasn't run yet.
-	 */
-	if (list->next == &rq->ipi_list)
-		raise_softirq_irqoff(BLOCK_SOFTIRQ);
-	local_irq_restore(flags);
+	blk_complete_reqs(this_cpu_ptr(&blk_cpu_done));
 }
 
 static int blk_softirq_cpu_dead(unsigned int cpu)
 {
-	/*
-	 * If a CPU goes away, splice its entries to the current CPU
-	 * and trigger a run of the softirq
-	 */
-	local_irq_disable();
-	list_splice_init(&per_cpu(blk_cpu_done, cpu),
-			 this_cpu_ptr(&blk_cpu_done));
-	raise_softirq_irqoff(BLOCK_SOFTIRQ);
-	local_irq_enable();
-
+	blk_complete_reqs(&per_cpu(blk_cpu_done, cpu));
 	return 0;
 }
 
-
 static void __blk_mq_complete_request_remote(void *data)
 {
-	struct request *rq = data;
-
-	blk_mq_trigger_softirq(rq);
+	__raise_softirq_irqoff(BLOCK_SOFTIRQ);
 }
 
 static inline bool blk_mq_complete_need_ipi(struct request *rq)
@@ -657,6 +618,28 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq)
 	return cpu_online(rq->mq_ctx->cpu);
 }
 
+static void blk_mq_complete_send_ipi(struct request *rq)
+{
+	unsigned int cpu = rq->mq_ctx->cpu;
+	struct llist_head *list = &per_cpu(blk_cpu_done, cpu);
+
+	if (llist_add(&rq->ipi_list, list)) {
+		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
+		smp_call_function_single_async(cpu, &rq->csd);
+	}
+}
+
+static void blk_mq_raise_softirq(struct request *rq)
+{
+	struct llist_head *list;
+
+	preempt_disable();
+	list = this_cpu_ptr(&blk_cpu_done);
+	if (llist_add(&rq->ipi_list, list))
+		raise_softirq(BLOCK_SOFTIRQ);
+	preempt_enable();
+}
+
 bool blk_mq_complete_request_remote(struct request *rq)
 {
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
@@ -669,15 +652,15 @@ bool blk_mq_complete_request_remote(struct request *rq)
 		return false;
 
 	if (blk_mq_complete_need_ipi(rq)) {
-		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
-		smp_call_function_single_async(rq->mq_ctx->cpu, &rq->csd);
-	} else {
-		if (rq->q->nr_hw_queues > 1)
-			return false;
-		blk_mq_trigger_softirq(rq);
+		blk_mq_complete_send_ipi(rq);
+		return true;
 	}
 
-	return true;
+	if (rq->q->nr_hw_queues == 1) {
+		blk_mq_raise_softirq(rq);
+		return true;
+	}
+	return false;
 }
 EXPORT_SYMBOL_GPL(blk_mq_complete_request_remote);
 
@@ -3892,7 +3875,7 @@ static int __init blk_mq_init(void)
 	int i;
 
 	for_each_possible_cpu(i)
-		INIT_LIST_HEAD(&per_cpu(blk_cpu_done, i));
+		init_llist_head(&per_cpu(blk_cpu_done, i));
 	open_softirq(BLOCK_SOFTIRQ, blk_done_softirq);
 
 	cpuhp_setup_state_nocalls(CPUHP_BLOCK_SOFTIRQ_DEAD,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f94ee3089e015..89a444c5a5833 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -153,7 +153,7 @@ struct request {
 	 */
 	union {
 		struct hlist_node hash;	/* merge hash */
-		struct list_head ipi_list;
+		struct llist_node ipi_list;
 	};
 
 	/*
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/3 v2] blk-mq: Use llist_head for blk_cpu_done
  2021-01-25  9:54         ` [PATCH 3/3 v2] " Sebastian Andrzej Siewior
@ 2021-01-25 10:14           ` Christoph Hellwig
  2021-01-27 11:23           ` Daniel Wagner
  1 sibling, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2021-01-25 10:14 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Christoph Hellwig, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq
  2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
  2021-01-25  7:10   ` Hannes Reinecke
  2021-01-25  8:22   ` Christoph Hellwig
@ 2021-01-27 11:22   ` Daniel Wagner
  2 siblings, 0 replies; 25+ messages in thread
From: Daniel Wagner @ 2021-01-27 11:22 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner,
	Peter Zijlstra, Ingo Molnar

On Sat, Jan 23, 2021 at 09:10:26PM +0100, Sebastian Andrzej Siewior wrote:
> Controllers with multiple queues have their IRQ-handelers pinned to a
> CPU. The core shouldn't need to complete the request on a remote CPU.
> 
> Remove this case and always raise the softirq to complete the request.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/3 v2] blk-mq: Use llist_head for blk_cpu_done
  2021-01-25  9:54         ` [PATCH 3/3 v2] " Sebastian Andrzej Siewior
  2021-01-25 10:14           ` Christoph Hellwig
@ 2021-01-27 11:23           ` Daniel Wagner
  1 sibling, 0 replies; 25+ messages in thread
From: Daniel Wagner @ 2021-01-27 11:23 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Christoph Hellwig, linux-block, linux-kernel, Jens Axboe,
	Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On Mon, Jan 25, 2021 at 10:54:12AM +0100, Sebastian Andrzej Siewior wrote:
> With llist_head it is possible to avoid the locking (the irq-off region)
> when items are added. This makes it possible to add items on a remote
> CPU without additional locking.
> llist_add() returns true if the list was previously empty. This can be
> used to invoke the SMP function call / raise sofirq only if the first
> item was added (otherwise it is already pending).
> This simplifies the code a little and reduces the IRQ-off regions.
> 
> blk_mq_raise_softirq() needs a preempt-disable section to ensure the
> request is enqueued on the same CPU as the softirq is raised.
> Some callers (USB-storage) invoke this path in preemptible context.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I did a quick test run with the whole series. Looks good.

Reviewed-by: Daniel Wagner <dwagner@suse.de>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
@ 2021-02-01 19:35   ` Sebastian Andrzej Siewior
  2021-02-09 10:02   ` Peter Zijlstra
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-01 19:35 UTC (permalink / raw)
  To: linux-block, linux-kernel
  Cc: Jens Axboe, Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 2021-01-23 21:10:25 [+0100], To linux-block@vger.kernel.org wrote:
> send_call_function_single_ipi() may wake an idle CPU without sending an
> IPI. The woken up CPU will process the SMP-functions in
> flush_smp_call_function_from_idle(). Any raised softirq from within the
> SMP-function call will not be processed.
> Should the CPU have no tasks assigned, then it will go back to idle with
> pending softirqs and the NOHZ will rightfully complain.
> 
> Process pending softirqs on return from flush_smp_call_function_queue().
> 
> Fixes: b2a02fc43a1f4 ("smp: Optimize send_call_function_single_ipi()")
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

A gentle ping.
This isn't just a requirement for the series: rps_trigger_softirq() is
invoked from smp_call_function_single_async() and raises a softirq.

Sebastian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
  2021-02-01 19:35   ` Sebastian Andrzej Siewior
@ 2021-02-09 10:02   ` Peter Zijlstra
  2021-02-09 11:35     ` Sebastian Andrzej Siewior
  2021-02-10 13:53   ` [tip: sched/core] " tip-bot2 for Sebastian Andrzej Siewior
  2021-02-17 13:17   ` tip-bot2 for Sebastian Andrzej Siewior
  3 siblings, 1 reply; 25+ messages in thread
From: Peter Zijlstra @ 2021-02-09 10:02 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner, Ingo Molnar

On Sat, Jan 23, 2021 at 09:10:25PM +0100, Sebastian Andrzej Siewior wrote:
> send_call_function_single_ipi() may wake an idle CPU without sending an
> IPI. The woken up CPU will process the SMP-functions in
> flush_smp_call_function_from_idle(). Any raised softirq from within the
> SMP-function call will not be processed.
> Should the CPU have no tasks assigned, then it will go back to idle with
> pending softirqs and the NOHZ will rightfully complain.
> 
> Process pending softirqs on return from flush_smp_call_function_queue().
> 
> Fixes: b2a02fc43a1f4 ("smp: Optimize send_call_function_single_ipi()")
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Fair enough. I'll stick this in tip/sched/smp for Jens and merge that
into tip/sched/core.

Thanks!

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-02-09 10:02   ` Peter Zijlstra
@ 2021-02-09 11:35     ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 25+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-02-09 11:35 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-block, linux-kernel, Jens Axboe, Thomas Gleixner, Ingo Molnar

On 2021-02-09 11:02:10 [+0100], Peter Zijlstra wrote:
> Fair enough. I'll stick this in tip/sched/smp for Jens and merge that
> into tip/sched/core.

Thank you.

> Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [tip: sched/core] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
  2021-02-01 19:35   ` Sebastian Andrzej Siewior
  2021-02-09 10:02   ` Peter Zijlstra
@ 2021-02-10 13:53   ` tip-bot2 for Sebastian Andrzej Siewior
  2021-02-17 13:17   ` tip-bot2 for Sebastian Andrzej Siewior
  3 siblings, 0 replies; 25+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2021-02-10 13:53 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Jens Axboe, Sebastian Andrzej Siewior, Peter Zijlstra (Intel),
	x86, linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     66040b2d5d41f85cb1a752a75260595344c5ec3b
Gitweb:        https://git.kernel.org/tip/66040b2d5d41f85cb1a752a75260595344c5ec3b
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Sat, 23 Jan 2021 21:10:25 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 10 Feb 2021 14:44:42 +01:00

smp: Process pending softirqs in flush_smp_call_function_from_idle()

send_call_function_single_ipi() may wake an idle CPU without sending an
IPI. The woken up CPU will process the SMP-functions in
flush_smp_call_function_from_idle(). Any raised softirq from within the
SMP-function call will not be processed.
Should the CPU have no tasks assigned, then it will go back to idle with
pending softirqs and the NOHZ will rightfully complain.

Process pending softirqs on return from flush_smp_call_function_queue().

Fixes: b2a02fc43a1f4 ("smp: Optimize send_call_function_single_ipi()")
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20210123201027.3262800-2-bigeasy@linutronix.de
---
 kernel/smp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/smp.c b/kernel/smp.c
index 1b6070b..aeb0adf 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -14,6 +14,7 @@
 #include <linux/export.h>
 #include <linux/percpu.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
@@ -449,6 +450,9 @@ void flush_smp_call_function_from_idle(void)
 
 	local_irq_save(flags);
 	flush_smp_call_function_queue(true);
+	if (local_softirq_pending())
+		do_softirq();
+
 	local_irq_restore(flags);
 }
 

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head
  2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
                   ` (3 preceding siblings ...)
  2021-01-25  4:27 ` [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Jens Axboe
@ 2021-02-10 14:43 ` Jens Axboe
  4 siblings, 0 replies; 25+ messages in thread
From: Jens Axboe @ 2021-02-10 14:43 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-block, linux-kernel
  Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar

On 1/23/21 1:10 PM, Sebastian Andrzej Siewior wrote:
> Patch 2+3 were applied and then dropped by Jens due to a NOHZ+softirq
> related warning [0]. Turns out a successful wakeup via
> set_nr_if_polling() will not process any softirqs and the CPU may go
> back to idle. This is addressed by patch #1.
> 
> smpcfd_dying_cpu() will also invoke SMP-functions calls via
> flush_smp_call_function_queue() but the block layer shouldn't queue
> anything because the CPU isn't online anymore.
> The two caller of flush_smp_call_function_from_idle() look fine with
> opening interrupts from within do_softirq().

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [tip: sched/core] smp: Process pending softirqs in flush_smp_call_function_from_idle()
  2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
                     ` (2 preceding siblings ...)
  2021-02-10 13:53   ` [tip: sched/core] " tip-bot2 for Sebastian Andrzej Siewior
@ 2021-02-17 13:17   ` tip-bot2 for Sebastian Andrzej Siewior
  3 siblings, 0 replies; 25+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2021-02-17 13:17 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Jens Axboe, Sebastian Andrzej Siewior, Peter Zijlstra (Intel),
	Ingo Molnar, x86, linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     f9d34595ae4feed38856b88769e2ba5af22d2548
Gitweb:        https://git.kernel.org/tip/f9d34595ae4feed38856b88769e2ba5af22d2548
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Sat, 23 Jan 2021 21:10:25 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 17 Feb 2021 14:12:42 +01:00

smp: Process pending softirqs in flush_smp_call_function_from_idle()

send_call_function_single_ipi() may wake an idle CPU without sending an
IPI. The woken up CPU will process the SMP-functions in
flush_smp_call_function_from_idle(). Any raised softirq from within the
SMP-function call will not be processed.
Should the CPU have no tasks assigned, then it will go back to idle with
pending softirqs and the NOHZ will rightfully complain.

Process pending softirqs on return from flush_smp_call_function_queue().

Fixes: b2a02fc43a1f4 ("smp: Optimize send_call_function_single_ipi()")
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lkml.kernel.org/r/20210123201027.3262800-2-bigeasy@linutronix.de
---
 kernel/smp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/smp.c b/kernel/smp.c
index 1b6070b..aeb0adf 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -14,6 +14,7 @@
 #include <linux/export.h>
 #include <linux/percpu.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/gfp.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
@@ -449,6 +450,9 @@ void flush_smp_call_function_from_idle(void)
 
 	local_irq_save(flags);
 	flush_smp_call_function_queue(true);
+	if (local_softirq_pending())
+		do_softirq();
+
 	local_irq_restore(flags);
 }
 

^ permalink raw reply related	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-02-17 13:20 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-23 20:10 [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Sebastian Andrzej Siewior
2021-01-23 20:10 ` [PATCH 1/3] smp: Process pending softirqs in flush_smp_call_function_from_idle() Sebastian Andrzej Siewior
2021-02-01 19:35   ` Sebastian Andrzej Siewior
2021-02-09 10:02   ` Peter Zijlstra
2021-02-09 11:35     ` Sebastian Andrzej Siewior
2021-02-10 13:53   ` [tip: sched/core] " tip-bot2 for Sebastian Andrzej Siewior
2021-02-17 13:17   ` tip-bot2 for Sebastian Andrzej Siewior
2021-01-23 20:10 ` [PATCH 2/3] blk-mq: Always complete remote completions requests in softirq Sebastian Andrzej Siewior
2021-01-25  7:10   ` Hannes Reinecke
2021-01-25  8:25     ` Christoph Hellwig
2021-01-25  8:30       ` Sebastian Andrzej Siewior
2021-01-25  8:32         ` Christoph Hellwig
2021-01-25  9:29           ` Sebastian Andrzej Siewior
2021-01-25  8:22   ` Christoph Hellwig
2021-01-25  8:49     ` Christoph Hellwig
2021-01-27 11:22   ` Daniel Wagner
2021-01-23 20:10 ` [PATCH 3/3] blk-mq: Use llist_head for blk_cpu_done Sebastian Andrzej Siewior
2021-01-25  8:30   ` Christoph Hellwig
2021-01-25  8:32     ` Sebastian Andrzej Siewior
2021-01-25  8:39       ` Christoph Hellwig
2021-01-25  9:54         ` [PATCH 3/3 v2] " Sebastian Andrzej Siewior
2021-01-25 10:14           ` Christoph Hellwig
2021-01-27 11:23           ` Daniel Wagner
2021-01-25  4:27 ` [PATCH v3 0/3] blk-mq: Don't complete in IRQ, use llist_head Jens Axboe
2021-02-10 14:43 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).