| Message ID | 20211123161813.326307-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc block cleanups | expand |
On 23/11/2021 17:18, Jens Axboe wrote: > + poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat), > + GFP_ATOMIC); Why not kcalloc()?
On 11/23/21 9:21 AM, Johannes Thumshirn wrote: > On 23/11/2021 17:18, Jens Axboe wrote: >> + poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat), >> + GFP_ATOMIC); > > Why not kcalloc()? Sure, can do.
On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote: > This is essentially never used, yet it's about 1/3rd of the total > queue size. Allocate it when needed, and don't embed it in the queue. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > --- > block/blk-mq.c | 20 ++++++++++++++++++-- > block/blk-stat.c | 6 ------ > block/blk-sysfs.c | 1 + > include/linux/blkdev.h | 9 +++++++-- > 4 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 20a6445f6a01..cb41c441aa8f 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); > /* Enable polling stats and return whether they were already enabled. */ > static bool blk_poll_stats_enable(struct request_queue *q) > { > - if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) || > - blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) > + struct blk_rq_stat *poll_stat; > + > + if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags)) > return true; Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for q->poll_stat now?
On 11/23/21 9:41 AM, Christoph Hellwig wrote: > On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote: >> This is essentially never used, yet it's about 1/3rd of the total >> queue size. Allocate it when needed, and don't embed it in the queue. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> --- >> block/blk-mq.c | 20 ++++++++++++++++++-- >> block/blk-stat.c | 6 ------ >> block/blk-sysfs.c | 1 + >> include/linux/blkdev.h | 9 +++++++-- >> 4 files changed, 26 insertions(+), 10 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 20a6445f6a01..cb41c441aa8f 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); >> /* Enable polling stats and return whether they were already enabled. */ >> static bool blk_poll_stats_enable(struct request_queue *q) >> { >> - if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) || >> - blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) >> + struct blk_rq_stat *poll_stat; >> + >> + if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags)) >> return true; > > Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for > q->poll_stat now? I think so: diff --git a/block/blk-mq.c b/block/blk-mq.c index f011fa3ebcc7..af4580bdf931 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -4593,7 +4593,7 @@ static bool blk_poll_stats_enable(struct request_queue *q) { struct blk_rq_stat *poll_stat; - if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags)) + if (q->poll_stat) return true; poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat), @@ -4602,7 +4602,7 @@ static bool blk_poll_stats_enable(struct request_queue *q) return false; spin_lock_irq(&q->stats->lock); - if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) { + if (q->poll_stat) { spin_unlock_irq(&q->stats->lock); kfree(poll_stat); return true; @@ -4620,8 +4620,7 @@ static void blk_mq_poll_stats_start(struct request_queue *q) * We don't arm the callback if polling stats are not enabled or the * callback is already active. */ - if (!test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) || - blk_stat_is_active(q->poll_cb)) + if (!q->poll_stat || blk_stat_is_active(q->poll_cb)) return; blk_stat_activate_msecs(q->poll_cb, 100); diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index e1b846ec58cb..c079be1c58a3 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -785,7 +785,7 @@ static void blk_release_queue(struct kobject *kobj) might_sleep(); - if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags)) + if (q->poll_stat) blk_stat_remove_callback(q, q->poll_cb); blk_stat_free_callback(q->poll_cb); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 3d558cb397d5..20cf877d6627 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -414,7 +414,6 @@ struct request_queue { #define QUEUE_FLAG_FUA 18 /* device supports FUA writes */ #define QUEUE_FLAG_DAX 19 /* device supports DAX */ #define QUEUE_FLAG_STATS 20 /* track IO start and completion times */ -#define QUEUE_FLAG_POLL_STATS 21 /* collecting stats for hybrid polling */ #define QUEUE_FLAG_REGISTERED 22 /* queue has been registered to a disk */ #define QUEUE_FLAG_QUIESCED 24 /* queue has been quiesced */ #define QUEUE_FLAG_PCI_P2PDMA 25 /* device supports PCI p2p requests */
On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote:
> I think so:
I think my eternal enemy in blk-mq-debugfs.c will need an update for
the flag removal, but otherwise this looks good.
On 11/23/21 10:05 AM, Christoph Hellwig wrote: > On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote: >> I think so: > > I think my eternal enemy in blk-mq-debugfs.c will need an update for > the flag removal, but otherwise this looks good. Heh yes, it always evades grep.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 20a6445f6a01..cb41c441aa8f 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); /* Enable polling stats and return whether they were already enabled. */ static bool blk_poll_stats_enable(struct request_queue *q) { - if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) || - blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) + struct blk_rq_stat *poll_stat; + + if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags)) return true; + + poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat), + GFP_ATOMIC); + if (!poll_stat) + return false; + + spin_lock_irq(&q->stats->lock); + if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) { + spin_unlock_irq(&q->stats->lock); + kfree(poll_stat); + return true; + } + q->poll_stat = poll_stat; + spin_unlock_irq(&q->stats->lock); + blk_stat_add_callback(q, q->poll_cb); return false; } diff --git a/block/blk-stat.c b/block/blk-stat.c index ae3dd1fb8e61..7ba504166d1b 100644 --- a/block/blk-stat.c +++ b/block/blk-stat.c @@ -12,12 +12,6 @@ #include "blk-mq.h" #include "blk.h" -struct blk_queue_stats { - struct list_head callbacks; - spinlock_t lock; - bool enable_accounting; -}; - void blk_rq_stat_init(struct blk_rq_stat *stat) { stat->min = -1ULL; diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index cd75b0f73dc6..e1b846ec58cb 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -790,6 +790,7 @@ static void blk_release_queue(struct kobject *kobj) blk_stat_free_callback(q->poll_cb); blk_free_queue_stats(q->stats); + kfree(q->poll_stat); blk_exit_queue(q); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index bd4370baccca..b46fd2a80062 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -28,10 +28,15 @@ struct blk_flush_queue; struct kiocb; struct pr_ops; struct rq_qos; -struct blk_queue_stats; struct blk_stat_callback; struct blk_crypto_profile; +struct blk_queue_stats { + struct list_head callbacks; + spinlock_t lock; + bool enable_accounting; +}; + /* Must be consistent with blk_mq_poll_stats_bkt() */ #define BLK_MQ_POLL_STATS_BKTS 16 @@ -267,7 +272,7 @@ struct request_queue { int poll_nsec; struct blk_stat_callback *poll_cb; - struct blk_rq_stat poll_stat[BLK_MQ_POLL_STATS_BKTS]; + struct blk_rq_stat *poll_stat; struct timer_list timeout; struct work_struct timeout_work;
This is essentially never used, yet it's about 1/3rd of the total queue size. Allocate it when needed, and don't embed it in the queue. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- block/blk-mq.c | 20 ++++++++++++++++++-- block/blk-stat.c | 6 ------ block/blk-sysfs.c | 1 + include/linux/blkdev.h | 9 +++++++-- 4 files changed, 26 insertions(+), 10 deletions(-)