Skip to contentSkip to conversation

nvme: remove cq check after submission

PATCH New Jens Axboe 3 participants 5 messages May 16, 2018 – May 17, 2018 NVMe

lore
Share & tools
lore

Jens Axboe

author maintainer
#0 lore raw

PATCH 1/1 nvme: remove cq check after submission

We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it.

Signed-off-by Jens Axboe <axboe at kernel.dk>
drivers/nvme/host/pci.c +0 -2
index 17a0190bd88f..79bbfadcb7b9 100644
@@ -68,7 +68,6 @@ MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");
 struct nvme_dev;
 struct nvme_queue;
 
-static void nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
 /*

@@ -895,7 +894,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 		goto out_cleanup_iod;
 	}
 	__nvme_submit_cmd(nvmeq, &cmnd);
-	nvme_process_cq(nvmeq);
 	spin_unlock_irq(&nvmeq->q_lock);
 	return BLK_STS_OK;
 out_cleanup_iod:
Signature · 1 line
Jens Axboe
#1 lore raw

PATCH 1/1 nvme: remove cq check after submission

Quoted 3 lines · Jens Axboe

We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it.

Right, this was on my todo list. Agreed it's not helping anything. Applied to nvme-4.18.

#2 lore raw

PATCH 1/1 nvme: remove cq check after submission

Quoted 7 lines · Keith Busch
Quoted 3 lines · Jens Axboe

We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it.

Right, this was on my todo list. Agreed it's not helping anything. Applied to nvme-4.18.

While I agree in general didn't we have a case where this caused a regression last time it came up? Not that I'd complain if it goes away..

But more importantly we could split q_lock into a sq_lock and cq_lock now, which might bring real benefits.

author maintainer
#3 lore raw

PATCH 1/1 nvme: remove cq check after submission

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Keith Busch
Quoted 3 lines · Jens Axboe

We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it.

Quoted 6 lines
Quoted 2 lines

Right, this was on my todo list. Agreed it's not helping anything. Applied to nvme-4.18.

While I agree in general didn't we have a case where this caused a regression last time it came up? Not that I'd complain if it goes away..

Not that I'm aware of. I've been running with this patch for quite a while, while doing some work on polling.

Quoted 2 lines

But more importantly we could split q_lock into a sq_lock and cq_lock now, which might bring real benefits.

Exactly, especially on queue starved setups.

Signature · 1 line
Jens Axboe
#4 lore raw

PATCH 1/1 nvme: remove cq check after submission

Quoted 10 lines · Jens Axboe
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · Keith Busch
Quoted 3 lines · Jens Axboe

We always check the completion queue after submitting, but in my testing this isn't a win even on DRAM/xpoint devices. In some cases it's actually worse. Kill it.

Right, this was on my todo list. Agreed it's not helping anything. Applied to nvme-4.18.

Quoted 6 lines
Quoted 3 lines

While I agree in general didn't we have a case where this caused a regression last time it came up? Not that I'd complain if it goes away..

Not that I'm aware of. I've been running with this patch for quite a while, while doing some work on polling.

Yah, I mentioned a few months back there appeared to be a problem with removing this, but I was mistaken. I just didn't get back around to doing this.