[PATCH 0/2] Some performance fixes for fastpath

[PATCH 0/2] Some performance fixes for fastpath

[Jon Derrick]

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(-)

-- 
1.8.3.1

[PATCH 1/2] Block: Disable polling stats when iostats are disabled

[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(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index ab51685..354d03b 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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;
 		}
-- 
1.8.3.1

[PATCH 1/2] Block: Disable polling stats when iostats are disabled

[Sagi Grimberg]

> 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(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ab51685..354d03b 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -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;
>   		}

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

[Jon Derrick]

> 
> 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

[Jens Axboe]

On 03/08/2016 10:34 AM, Jon Derrick wrote:
> 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(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ab51685..354d03b 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -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;
>   		}
>

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.

-- 
Jens Axboe

[PATCH 1/2] Block: Disable polling stats when iostats are disabled

[Jon Derrick]

> 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

> 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

[Jon Derrick]

Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
---
 drivers/nvme/host/pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 74514c7..e9f18e1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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;
-- 
1.8.3.1

[PATCH 2/2] NVMe: Remove unused sq_head read in completion path

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

[PATCH 2/2] NVMe: Remove unused sq_head read in completion path

[Jens Axboe]

On 03/08/2016 10:34 AM, Jon Derrick wrote:
> Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
> ---
>   drivers/nvme/host/pci.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 74514c7..e9f18e1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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;

This seems like a no-brainer, good spotting.

-- 
Jens Axboe

[PATCH 2/2] NVMe: Remove unused sq_head read in completion path

[Johannes Thumshirn]

On Tue, Mar 08, 2016@10:34:54AM -0700, Jon Derrick wrote:
> Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
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