diff mbox series

[3/3] block: only allocate poll_stats if there's a user of them

Message ID 20211123171058.346084-4-axboe@kernel.dk (mailing list archive)
State New, archived
Headers show
Series Misc block cleanups | expand

Commit Message

Jens Axboe Nov. 23, 2021, 5:10 p.m. UTC 
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.

Kill the queue flag for this while at it, since we can just check the
assigned pointer now.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq-debugfs.c |  1 -
 block/blk-mq.c         | 23 +++++++++++++++++++----
 block/blk-stat.c       |  6 ------
 block/blk-sysfs.c      |  3 ++-
 include/linux/blkdev.h | 10 +++++++---
 5 files changed, 28 insertions(+), 15 deletions(-)

Comments

Christoph Hellwig Nov. 23, 2021, 6:50 p.m. UTC | #1 
> +	spin_lock_irq(&q->stats->lock);
> +	if (q->poll_stat) {
> +		spin_unlock_irq(&q->stats->lock);
> +		kfree(poll_stat);
> +		return true;
> +	}
> +	q->poll_stat = poll_stat;
> +	spin_unlock_irq(&q->stats->lock);

If we'd use a cmpxchg to install the pointer we could keep the
blk_queue_stats definition private.
Jens Axboe Nov. 23, 2021, 6:58 p.m. UTC | #2 
On 11/23/21 11:50 AM, Christoph Hellwig wrote:
>> +	spin_lock_irq(&q->stats->lock);
>> +	if (q->poll_stat) {
>> +		spin_unlock_irq(&q->stats->lock);
>> +		kfree(poll_stat);
>> +		return true;
>> +	}
>> +	q->poll_stat = poll_stat;
>> +	spin_unlock_irq(&q->stats->lock);
> 
> If we'd use a cmpxchg to install the pointer we could keep the
> blk_queue_stats definition private.

How about we just move this alloc+enable logic into blk-stat.c instead?
Christoph Hellwig Nov. 23, 2021, 6:59 p.m. UTC | #3 
On Tue, Nov 23, 2021 at 11:58:42AM -0700, Jens Axboe wrote:
> On 11/23/21 11:50 AM, Christoph Hellwig wrote:
> >> +	spin_lock_irq(&q->stats->lock);
> >> +	if (q->poll_stat) {
> >> +		spin_unlock_irq(&q->stats->lock);
> >> +		kfree(poll_stat);
> >> +		return true;
> >> +	}
> >> +	q->poll_stat = poll_stat;
> >> +	spin_unlock_irq(&q->stats->lock);
> > 
> > If we'd use a cmpxchg to install the pointer we could keep the
> > blk_queue_stats definition private.
> 
> How about we just move this alloc+enable logic into blk-stat.c instead?

That's a good idea either way.  But I think cmpxchg is much better
for installing a pointer than an unrelated lock.
Jens Axboe Nov. 23, 2021, 7:03 p.m. UTC | #4 
On 11/23/21 11:59 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 11:58:42AM -0700, Jens Axboe wrote:
>> On 11/23/21 11:50 AM, Christoph Hellwig wrote:
>>>> +	spin_lock_irq(&q->stats->lock);
>>>> +	if (q->poll_stat) {
>>>> +		spin_unlock_irq(&q->stats->lock);
>>>> +		kfree(poll_stat);
>>>> +		return true;
>>>> +	}
>>>> +	q->poll_stat = poll_stat;
>>>> +	spin_unlock_irq(&q->stats->lock);
>>>
>>> If we'd use a cmpxchg to install the pointer we could keep the
>>> blk_queue_stats definition private.
>>
>> How about we just move this alloc+enable logic into blk-stat.c instead?
> 
> That's a good idea either way.  But I think cmpxchg is much better
> for installing a pointer than an unrelated lock.

True, I'll do that while moving it.
diff mbox series

Patch

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 4f2cf8399f3d..f4022b198580 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -122,7 +122,6 @@  static const char *const blk_queue_flag_name[] = {
 	QUEUE_FLAG_NAME(FUA),
 	QUEUE_FLAG_NAME(DAX),
 	QUEUE_FLAG_NAME(STATS),
-	QUEUE_FLAG_NAME(POLL_STATS),
 	QUEUE_FLAG_NAME(REGISTERED),
 	QUEUE_FLAG_NAME(QUIESCED),
 	QUEUE_FLAG_NAME(PCI_P2PDMA),
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 20a6445f6a01..4c59b24690d7 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 (q->poll_stat)
 		return true;
+
+	poll_stat = kcalloc(BLK_MQ_POLL_STATS_BKTS, sizeof(*poll_stat),
+				GFP_ATOMIC);
+	if (!poll_stat)
+		return false;
+
+	spin_lock_irq(&q->stats->lock);
+	if (q->poll_stat) {
+		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;
 }
@@ -4590,8 +4606,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-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..c079be1c58a3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -785,11 +785,12 @@  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);
 
 	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..90dac4a67cfc 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;
@@ -397,7 +402,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 */