Some performance fixes for fastpath
Subsystems (2)
cover Some performance fixes for fastpath
This is a small set of hopefully uncontroversial, performance-related patches. Local testing on new hardware showed a 5-10MBps increase in 4k and 1M sequential synchronous null reads from an NVMe device.
Jon Derrick (2):
Block: Disable polling stats when iostats are disabled
NVMe: Remove unused sq_head read in completion path
block/blk-core.c | 7 +++++-- drivers/nvme/host/pci.c | 2 -- 2 files changed, 5 insertions(+), 4 deletions(-)
Signature · 1 line
1.8.3.1
PATCH 1/2 Block: Disable polling stats when iostats are disabled
Extends iostats to encompass polling statistics to save a few cycles when disabled.
--- block/blk-core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
block/blk-core.c +5 -2
@@ -3350,13 +3350,16 @@ bool blk_poll(struct request_queue *q, blk_qc_t cookie)while (!need_resched ()) {unsigned int queue_num =blk_qc_t_to_queue_num (cookie);struct blk_mq_hw_ctx *hctx = q->queue_hw_ctx[queue_num]; +int io_stat =blk_queue_io_stat (q);int ret; - hctx->poll_invoked++; +if (io_stat) + hctx->poll_invoked++; ret = q->mq_ops->poll (hctx,blk_qc_t_to_tag (cookie));if (ret >0 ) { - hctx->poll_success++; +if (io_stat) + hctx->poll_success++;set_current_state (TASK_RUNNING );return true; }
Signature · 1 line
1.8.3.1
PATCH 1/2 Block: Disable polling stats when iostats are disabled
Quoted 22 lines
Extends iostats to encompass polling statistics to save a few cycles when disabled.
Signed-off-by Jon Derrick <jonathan.derrick at intel.com>--- block/blk-core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)block/blk-core.c +2
-1@@ -3350,13 +3350,16 @@ bool blk_poll(struct request_queue *q, blk_qc_t cookie)while (!need_resched ()) {unsigned int queue_num =blk_qc_t_to_queue_num (cookie);struct blk_mq_hw_ctx *hctx = q->queue_hw_ctx[queue_num]; +int io_stat =blk_queue_io_stat (q);int ret; - hctx->poll_invoked++; +if (io_stat)
block/blk-core.c bool blk_poll(struct request_queue *q, blk_qc_t cookie) · line 3365
Quoted 10 lines
block/blk-core.c +3
-1+ hctx->poll_invoked++; ret = q->mq_ops->poll (hctx,blk_qc_t_to_tag (cookie));if (ret >0 ) { - hctx->poll_success++; +if (io_stat) + hctx->poll_success++;set_current_state (TASK_RUNNING );return true; }
I fail to see how replacing incrementation with a branch statement helps performance or even not making it worse...
PATCH 1/2 Block: Disable polling stats when iostats are disabled
Quoted 3 lines
I fail to see how replacing incrementation with a branch statement helps performance or even not making it worse...
It may have been coincidental in my setup - I had assumed the test on the local io_stat was cheaper than the cost of following hctx to then increment (or not) poll_invoked.
PATCH 1/2 Block: Disable polling stats when iostats are disabled
In reply to Jon DerrickQuoted 22 lines · Jon Derrick
Extends iostats to encompass polling statistics to save a few cycles when disabled.
Signed-off-by Jon Derrick <jonathan.derrick at intel.com>--- block/blk-core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)block/blk-core.c +2
-1@@ -3350,13 +3350,16 @@ bool blk_poll(struct request_queue *q, blk_qc_t cookie)while (!need_resched ()) {unsigned int queue_num =blk_qc_t_to_queue_num (cookie);struct blk_mq_hw_ctx *hctx = q->queue_hw_ctx[queue_num]; +int io_stat =blk_queue_io_stat (q);int ret; - hctx->poll_invoked++; +if (io_stat)
block/blk-core.c bool blk_poll(struct request_queue *q, blk_qc_t cookie) · line 3365
Quoted 10 lines
block/blk-core.c +3
-1+ hctx->poll_invoked++; ret = q->mq_ops->poll (hctx,blk_qc_t_to_tag (cookie));if (ret >0 ) { - hctx->poll_success++; +if (io_stat) + hctx->poll_success++;set_current_state (TASK_RUNNING );return true; }
Not sure this is a great idea. First of all, the poll stats are per hardware queue. How many submission queues and CPUs do you have in your setup? For most cases, I'd assume there'd be a 1:1 mapping between the two, which makes the stats essentially free. And secondly, even for a less optimal mapping, the poll stats are a lot cheaper than the io stats. So bundling them together might not make a ton of sense.
Signature · 1 line
Jens Axboe
PATCH 1/2 Block: Disable polling stats when iostats are disabled
Quoted 2 lines
Not sure this is a great idea. First of all, the poll stats are per hardware queue. How many submission queues and CPUs do you have in your setup? For
31 hq, 64 cpus (so actually just 31 sq)- but the test I ran was:
taskset -c 1 fio --name=global --gtod_reduce=1 --filename=/dev/nvme0n1 --bs=4k --rw=read --ioengine=sync --iodepth=1 --numjobs=1 --direct=1 --name=job
I was a bit surprised at the delta I saw, but it may have been coincidental
Quoted 4 lines
most cases, I'd assume there'd be a 1:1 mapping between the two, which makes the stats essentially free. And secondly, even for a less optimal mapping, the poll stats are a lot cheaper than the io stats. So bundling them together might not make a ton of sense.
Fair enough :)
PATCH 2/2 NVMe: Remove unused sq_head read in completion path
In reply to Jon Derrick--- drivers/nvme/host/pci.c | 2 -- 1 file changed, 2 deletions(-)
drivers/nvme/host/pci.c +0 -2
@@ -134,7 +134,6 @@ struct nvme_queue {u32 __iomem *q_db;u16 q_depth;s16 cq_vector; -u16 sq_head;u16 sq_tail;u16 cq_head;u16 qid;
@@ -719,7 +718,6 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)if ((status &1 ) != phase)break ; - nvmeq->sq_head =le16_to_cpu (cqe.sq_head);if (++head == nvmeq->q_depth) { head =0 ; phase = !phase;
Signature · 1 line
1.8.3.1
PATCH 2/2 NVMe: Remove unused sq_head read in completion path
Looks fine,
PATCH 2/2 NVMe: Remove unused sq_head read in completion path
In reply to Jon DerrickQuoted 18 lines · Jon Derrick
Signed-off-by Jon Derrick <jonathan.derrick at intel.com>--- drivers/nvme/host/pci.c | 2 -- 1 file changed, 2 deletions(-)drivers/nvme/host/pci.c +0
-1@@ -134,7 +134,6 @@ struct nvme_queue {u32 __iomem *q_db;u16 q_depth;s16 cq_vector; -u16 sq_head;u16 sq_tail;u16 cq_head;u16 qid;
drivers/nvme/host/pci.c static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag) · line 723
Quoted 7 lines
drivers/nvme/host/pci.c +0
-1@@ -719,7 +718,6 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)if ((status &1 ) != phase)break ; - nvmeq->sq_head =le16_to_cpu (cqe.sq_head);if (++head == nvmeq->q_depth) { head =0 ; phase = !phase;
This seems like a no-brainer, good spotting.
Signature · 1 line
Jens Axboe
PATCH 2/2 NVMe: Remove unused sq_head read in completion path
In reply to Jon DerrickQuoted 1 line · Jon Derrick
Signed-off-by Jon Derrick <jonathan.derrick at intel.com>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850