[PATCH] nvme: remove cq check after submission

[PATCH] nvme: remove cq check after submission

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

Signed-off-by: Jens Axboe <axboe at kernel.dk>

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 17a0190bd88f..79bbfadcb7b9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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:

-- 
Jens Axboe

[PATCH] nvme: remove cq check after submission

[Keith Busch]

On Wed, May 16, 2018@01:58:08PM -0600, Jens Axboe wrote:
> 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.

[PATCH] nvme: remove cq check after submission

[Christoph Hellwig]

On Wed, May 16, 2018@03:20:10PM -0600, Keith Busch wrote:
> On Wed, May 16, 2018@01:58:08PM -0600, Jens Axboe wrote:
> > 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.

[PATCH] nvme: remove cq check after submission

[Jens Axboe]

On 5/17/18 12:50 AM, Christoph Hellwig wrote:
> On Wed, May 16, 2018@03:20:10PM -0600, Keith Busch wrote:
>> On Wed, May 16, 2018@01:58:08PM -0600, Jens Axboe wrote:
>>> 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..

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

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

-- 
Jens Axboe

[PATCH] nvme: remove cq check after submission

[Keith Busch]

On Thu, May 17, 2018@08:16:17AM -0600, Jens Axboe wrote:
> On 5/17/18 12:50 AM, Christoph Hellwig wrote:
> > On Wed, May 16, 2018@03:20:10PM -0600, Keith Busch wrote:
> >> On Wed, May 16, 2018@01:58:08PM -0600, Jens Axboe wrote:
> >>> 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..
> 
> 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.