linux-kernel - Re: panic with CPU hotplug + blk-mq + scsi-mq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150420183645.GA25549@posteo.de>
Date:	Mon, 20 Apr 2015 20:36:45 +0200
From:	Dongsu Park <dongsu.park@...fitbricks.com>
To:	Ming Lei <ming.lei@...onical.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jens Axboe <axboe@...nel.dk>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: panic with CPU hotplug + blk-mq + scsi-mq

On 21.04.2015 00:48, Ming Lei wrote:
> Thanks for providing that.
> The trick is just in CPU number and virito-scsi hw queue number,
> and that is why I asked that, :-)
> Now the problem is quite clear, before CPU1 online, suppose
> CPU3 is mapped hw queue 6, and CPU 3 will map to hw queue 5
> after CPU1 is offline, unfortunately current code can't allocate
> tags for hw queue 5 even it becomes mapped.
> The following updated patch(include original patch 2) will fix
> the problem, and patch 1 is required too.
> So the following patch should fix your hotplug issue.

Yes, it works indeed. Thanks a lot! :-)
You can add:

Tested-by: Dongsu Park <dongsu.park@...fitbricks.com>

As the original patch didn't apply, I had to change some nitpicks though.
(see below)

Cheers,
Dongsu

----

>From 8c0edcbbdfbab67dc8ae2fd46cca6a86e0cadcba Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@...onical.com>
Date: Sun, 19 Apr 2015 23:32:46 +0800
Subject: [PATCH v1 2/2] blk-mq: fix CPU hotplug handling

Firstly the hctx->tags have to be set as NULL if it is to be disabled
no matter if set->tags[i] is NULL or not in blk_mq_map_swqueue() because
shared tags can be freed already from another request queue.

The same situation has to be considered in blk_mq_hctx_cpu_online() too.

Finally one unmapped hw queue can be remapped after CPU topo is changed,
we need to allocate tags for the hw queue in blk_mq_map_swqueue() too.
Then tags allocation for hw queue can be removed in hctx cpu online
notifier, and it is reasonable to do that after remapping is done.

Cc: <stable@...r.kernel.org>
Reported-by: Dongsu Park <dongsu.park@...fitbricks.com>
Signed-off-by: Ming Lei <ming.lei@...onical.com>
---
 block/blk-mq.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 078840ce8670..df4b9597e477 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1573,22 +1573,6 @@ static int blk_mq_hctx_cpu_offline(struct blk_mq_hw_ctx *hctx, int cpu)
 	return NOTIFY_OK;
 }
 
-static int blk_mq_hctx_cpu_online(struct blk_mq_hw_ctx *hctx, int cpu)
-{
-	struct request_queue *q = hctx->queue;
-	struct blk_mq_tag_set *set = q->tag_set;
-
-	if (set->tags[hctx->queue_num])
-		return NOTIFY_OK;
-
-	set->tags[hctx->queue_num] = blk_mq_init_rq_map(set, hctx->queue_num);
-	if (!set->tags[hctx->queue_num])
-		return NOTIFY_STOP;
-
-	hctx->tags = set->tags[hctx->queue_num];
-	return NOTIFY_OK;
-}
-
 static int blk_mq_hctx_notify(void *data, unsigned long action,
 			      unsigned int cpu)
 {
@@ -1596,8 +1580,11 @@ static int blk_mq_hctx_notify(void *data, unsigned long action,
 
 	if (action == CPU_DEAD || action == CPU_DEAD_FROZEN)
 		return blk_mq_hctx_cpu_offline(hctx, cpu);
-	else if (action == CPU_ONLINE || action == CPU_ONLINE_FROZEN)
-		return blk_mq_hctx_cpu_online(hctx, cpu);
+
+	/*
+	 * In case of CPU online, tags will be reallocated
+	 * after new mapping is done in blk_mq_map_swqueue().
+	 */
 
 	return NOTIFY_OK;
 }
@@ -1779,6 +1766,7 @@ static void blk_mq_map_swqueue(struct request_queue *q)
 	unsigned int i;
 	struct blk_mq_hw_ctx *hctx;
 	struct blk_mq_ctx *ctx;
+	struct blk_mq_tag_set *set = q->tag_set;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		cpumask_clear(hctx->cpumask);
@@ -1805,16 +1793,20 @@ static void blk_mq_map_swqueue(struct request_queue *q)
 		 * disable it and free the request entries.
 		 */
 		if (!hctx->nr_ctx) {
-			struct blk_mq_tag_set *set = q->tag_set;
-
 			if (set->tags[i]) {
 				blk_mq_free_rq_map(set, set->tags[i], i);
 				set->tags[i] = NULL;
-				hctx->tags = NULL;
 			}
+			hctx->tags = NULL;
 			continue;
 		}
 
+		/* unmapped hw queue can be remapped after CPU topo changed */
+		if (!set->tags[i])
+			set->tags[i] = blk_mq_init_rq_map(set, hctx->queue_num);
+		hctx->tags = set->tags[i];
+		WARN_ON(!hctx->tags);
+
 		/*
 		 * Initialize batch roundrobin counts
 		 */
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/