[v3,6/7] block: refactor __blk_mq_alloc_rq_map_and_requests
diff mbox series

Message ID eb995b7a9c5a942c596eb21856043c4a1b2135f1.1586199103.git.zhangweiping@didiglobal.com
State New
Headers show
Series
  • Fix potential kernel panic when increase hardware queue
Related show

Commit Message

Weiping Zhang April 6, 2020, 7:37 p.m. UTC
This patch add a new member nr_allocated_map_rqs to the
struct blk_mq_tag_set to record the number of maps and requests have
been allocated for this tagset.

Now there is a problem when we increase hardware queue count, we do not
allocate maps and request for the new allocated hardware queue, it will
be fixed in the next patch.

Since request needs lots of memory, it's not easy alloc so many memory
dynamically, espeicially when system is under memory pressure.

This patch allow nr_hw_queues does not equal to the nr_allocated_map_rqs,
to avoid alloc/free memory when change hardware queue count.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 block/blk-mq.c         | 28 +++++++++++++++++++++-------
 include/linux/blk-mq.h |  1 +
 2 files changed, 22 insertions(+), 7 deletions(-)

Comments

Bart Van Assche April 20, 2020, 9:07 p.m. UTC | #1
On 4/6/20 12:37 PM, Weiping Zhang wrote:
> This patch add a new member nr_allocated_map_rqs to the
> struct blk_mq_tag_set to record the number of maps and requests have
> been allocated for this tagset.
> 
> Now there is a problem when we increase hardware queue count, we do not
> allocate maps and request for the new allocated hardware queue, it will
> be fixed in the next patch.
> 
> Since request needs lots of memory, it's not easy alloc so many memory
> dynamically, espeicially when system is under memory pressure.
> 
> This patch allow nr_hw_queues does not equal to the nr_allocated_map_rqs,
> to avoid alloc/free memory when change hardware queue count.

It seems to me that patches 6 and 7 combined fix a single issue. How 
about combining these two patches into a single patch?

Thanks,

Bart.
weiping zhang April 21, 2020, 1:24 p.m. UTC | #2
On Mon, Apr 20, 2020 at 02:07:13PM -0700, Bart Van Assche wrote:
> On 4/6/20 12:37 PM, Weiping Zhang wrote:
> >This patch add a new member nr_allocated_map_rqs to the
> >struct blk_mq_tag_set to record the number of maps and requests have
> >been allocated for this tagset.
> >
> >Now there is a problem when we increase hardware queue count, we do not
> >allocate maps and request for the new allocated hardware queue, it will
> >be fixed in the next patch.
> >
> >Since request needs lots of memory, it's not easy alloc so many memory
> >dynamically, espeicially when system is under memory pressure.
> >
> >This patch allow nr_hw_queues does not equal to the nr_allocated_map_rqs,
> >to avoid alloc/free memory when change hardware queue count.
> 
> It seems to me that patches 6 and 7 combined fix a single issue. How
> about combining these two patches into a single patch?
> 
It's ok for me, the reason why I split two patches is that make patch
review more easily, since patch-6 actually does not change original
logic. If you think combined them is more better, I can merge these
two patches.

Thanks
> Thanks,
> 
> Bart.

Patch
diff mbox series

diff --git a/block/blk-mq.c b/block/blk-mq.c
index df243c19a158..15f6a811122a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2479,8 +2479,10 @@  static bool __blk_mq_alloc_rq_map_and_request(struct blk_mq_tag_set *set, int hc
 
 	ret = blk_mq_alloc_rqs(set, set->tags[hctx_idx], hctx_idx,
 				set->queue_depth);
-	if (!ret)
+	if (!ret) {
+		set->nr_allocated_map_rqs++;
 		return true;
+	}
 
 	blk_mq_free_rq_map(set->tags[hctx_idx]);
 	set->tags[hctx_idx] = NULL;
@@ -2494,6 +2496,7 @@  static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set,
 		blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx);
 		blk_mq_free_rq_map(set->tags[hctx_idx]);
 		set->tags[hctx_idx] = NULL;
+		set->nr_allocated_map_rqs--;
 	}
 }
 
@@ -2978,18 +2981,28 @@  void blk_mq_exit_queue(struct request_queue *q)
 	blk_mq_exit_hw_queues(q, set, set->nr_hw_queues);
 }
 
-static int __blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set)
+/*
+ * Only append new map and requests, if new > now, all of these maps and
+ * request will be released when cleanup whole tag set. Because requests
+ * will cost lots memory, if system's memory is under a pressure, it's not
+ * easy to allocate too much memory.
+ */
+static int blk_mq_realloc_rq_map_and_requests(struct blk_mq_tag_set *set,
+						int new)
 {
-	int i;
+	int i, now = set->nr_allocated_map_rqs;
+
+	if (new <= now)
+		return 0;
 
-	for (i = 0; i < set->nr_hw_queues; i++)
+	for (i = now; i < new; i++)
 		if (!__blk_mq_alloc_rq_map_and_request(set, i))
 			goto out_unwind;
 
 	return 0;
 
 out_unwind:
-	while (--i >= 0)
+	while (--i >= now)
 		blk_mq_free_map_and_requests(set, i);
 
 	return -ENOMEM;
@@ -3007,7 +3020,8 @@  static int blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set)
 
 	depth = set->queue_depth;
 	do {
-		err = __blk_mq_alloc_rq_map_and_requests(set);
+		err = blk_mq_realloc_rq_map_and_requests(set,
+						set->nr_hw_queues);
 		if (!err)
 			break;
 
@@ -3184,7 +3198,7 @@  void blk_mq_free_tag_set(struct blk_mq_tag_set *set)
 {
 	int i, j;
 
-	for (i = 0; i < set->nr_hw_queues; i++)
+	for (i = 0; i < set->nr_allocated_map_rqs; i++)
 		blk_mq_free_map_and_requests(set, i);
 
 	for (j = 0; j < set->nr_maps; j++) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index f389d7c724bd..d950435cd3c6 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -240,6 +240,7 @@  struct blk_mq_tag_set {
 	unsigned int		nr_maps;
 	const struct blk_mq_ops	*ops;
 	unsigned int		nr_hw_queues;
+	unsigned int		nr_allocated_map_rqs;
 	unsigned int		queue_depth;
 	unsigned int		reserved_tags;
 	unsigned int		cmd_size;