| Message ID | e6368cfce3dca5238e8546e5624bbcab17824083.1586199103.git.zhangweiping@didiglobal.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix potential kernel panic when increase hardware queue | expand |
On 4/6/20 12:36 PM, Weiping Zhang wrote: > For this error handle, it should free both map and request, > otherwise memleak occur. ^^^^^^^ Please expand this into "a memory leak". > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 4692e8232699..406df9ce9b55 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2990,7 +2990,7 @@ static int __blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set) > > out_unwind: > while (--i >= 0) > - blk_mq_free_rq_map(set->tags[i]); > + blk_mq_free_map_and_requests(set, i); > > return -ENOMEM; > } The current upstream implementation is as follows: static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) { int i; for (i = 0; i < set->nr_hw_queues; i++) if (!__blk_mq_alloc_rq_map(set, i)) goto out_unwind; return 0; out_unwind: while (--i >= 0) blk_mq_free_rq_map(set->tags[i]); return -ENOMEM; } The pointers to the memory allocated by __blk_mq_alloc_rq_map() are stored in set->tags[i] and set->tags[i]->static_rqs. blk_mq_free_rq_map() frees set->tags[i]->static_rqs and set->tags[i]. In other words, I don't see which memory is leaked. What did I miss? Thanks, Bart.
On Mon, Apr 20, 2020 at 01:58:54PM -0700, Bart Van Assche wrote: > On 4/6/20 12:36 PM, Weiping Zhang wrote: > >For this error handle, it should free both map and request, > >otherwise memleak occur. > ^^^^^^^ > Please expand this into "a memory leak". > > >diff --git a/block/blk-mq.c b/block/blk-mq.c > >index 4692e8232699..406df9ce9b55 100644 > >--- a/block/blk-mq.c > >+++ b/block/blk-mq.c > >@@ -2990,7 +2990,7 @@ static int __blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set) > > out_unwind: > > while (--i >= 0) > >- blk_mq_free_rq_map(set->tags[i]); > >+ blk_mq_free_map_and_requests(set, i); > > return -ENOMEM; > > } > > The current upstream implementation is as follows: > > static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) > { > int i; > > for (i = 0; i < set->nr_hw_queues; i++) > if (!__blk_mq_alloc_rq_map(set, i)) > goto out_unwind; > > return 0; > > out_unwind: > while (--i >= 0) > blk_mq_free_rq_map(set->tags[i]); > > return -ENOMEM; > } > > The pointers to the memory allocated by __blk_mq_alloc_rq_map() are > stored in set->tags[i] and set->tags[i]->static_rqs. > blk_mq_free_rq_map() frees set->tags[i]->static_rqs and > set->tags[i]. In other words, I don't see which memory is leaked. > What did I miss? Hi Bart, __blk_mq_alloc_rq_map blk_mq_alloc_rq_map blk_mq_alloc_rq_map tags = blk_mq_init_tags : kzalloc_node: tags->rqs = kcalloc_node tags->static_rqs = kcalloc_node blk_mq_alloc_rqs p = alloc_pages_node tags->static_rqs[i] = p + offset; but blk_mq_free_rq_map kfree(tags->rqs); kfree(tags->static_rqs); blk_mq_free_tags kfree(tags); The page allocated in blk_mq_alloc_rqs cannot be released, so we should use blk_mq_free_map_and_requests here, blk_mq_free_map_and_requests blk_mq_free_rqs __free_pages : cleanup for blk_mq_alloc_rqs blk_mq_free_rq_map : cleanup for blk_mq_alloc_rq_map ... The reason why I rename some functions in previous three patches is that, these function not only allocate rq_map but also requests. How about rename them like this: __blk_mq_alloc_rq_map => __blk_mq_alloc_map_and_request __blk_mq_alloc_rq_maps => __blk_mq_alloc_map_and_requests, blk_mq_alloc_rq_maps => blk_mq_alloc_map_and_requests Thanks > > Thanks, > > Bart.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 4692e8232699..406df9ce9b55 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2990,7 +2990,7 @@ static int __blk_mq_alloc_rq_map_and_requests(struct blk_mq_tag_set *set) out_unwind: while (--i >= 0) - blk_mq_free_rq_map(set->tags[i]); + blk_mq_free_map_and_requests(set, i); return -ENOMEM; }
For this error handle, it should free both map and request, otherwise memleak occur. Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com> --- block/blk-mq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)