[PATCH 1/6] nvme: don't disable local ints for polled queue

[PATCH 1/6] nvme: don't disable local ints for polled queue

[Jens Axboe]

A polled queued doesn't trigger interrupts, so it's always safe
to grab the queue lock without disabling interrupts.

Cc: Keith Busch <keith.busch at intel.com>
Cc: linux-nvme at lists.infradead.org
Signed-off-by: Jens Axboe <axboe at kernel.dk>
---
 drivers/nvme/host/pci.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6aa86dfcb32c..a6e3fbddfadf 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1061,15 +1061,26 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 
 static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
 {
+	unsigned long flags = 0; /* gcc 7.x fail */
 	u16 start, end;
-	bool found;
+	bool found, has_irq;
 
 	if (!nvme_cqe_pending(nvmeq))
 		return 0;
 
-	spin_lock_irq(&nvmeq->cq_lock);
+	/*
+	 * Polled queue doesn't have an IRQ, no need to disable ints
+	 */
+	has_irq = !nvmeq->polled;
+	if (has_irq)
+		local_irq_save(flags);
+
+	spin_lock(&nvmeq->cq_lock);
 	found = nvme_process_cq(nvmeq, &start, &end, tag);
-	spin_unlock_irq(&nvmeq->cq_lock);
+	spin_unlock(&nvmeq->cq_lock);
+
+	if (has_irq)
+		local_irq_restore(flags);
 
 	nvme_complete_cqes(nvmeq, start, end);
 	return found;
-- 
2.17.1

[PATCH 1/6] nvme: don't disable local ints for polled queue

[Christoph Hellwig]

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6aa86dfcb32c..a6e3fbddfadf 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1061,15 +1061,26 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>  
>  static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
>  {
> +	unsigned long flags = 0; /* gcc 7.x fail */
>  	u16 start, end;
> -	bool found;
> +	bool found, has_irq;
>  
>  	if (!nvme_cqe_pending(nvmeq))
>  		return 0;
>  
> -	spin_lock_irq(&nvmeq->cq_lock);
> +	/*
> +	 * Polled queue doesn't have an IRQ, no need to disable ints
> +	 */
> +	has_irq = !nvmeq->polled;
> +	if (has_irq)
> +		local_irq_save(flags);
> +
> +	spin_lock(&nvmeq->cq_lock);
>  	found = nvme_process_cq(nvmeq, &start, &end, tag);
> -	spin_unlock_irq(&nvmeq->cq_lock);
> +	spin_unlock(&nvmeq->cq_lock);
> +
> +	if (has_irq)
> +		local_irq_restore(flags);

Eww, this just looks ugly.  Given that we are micro-optimizing here
I'd rather just use a different ->poll implementation and thus blk_mq_ops
if we have a separate poll code.  Then we could leave the existing
__nvme_poll as-is and have a new nvme_poll_noirq something like this:

static int nvme_poll_noirq(struct blk_mq_hw_ctx *hctx, unsigned int tag)
{
        struct nvme_queue *nvmeq = hctx->driver_data;
>  	u16 start, end;
	bool found;
>  
>  	if (!nvme_cqe_pending(nvmeq))
>  		return 0;
>  
> +	spin_lock(&nvmeq->cq_lock);
>  	found = nvme_process_cq(nvmeq, &start, &end, tag);
> +	spin_unlock(&nvmeq->cq_lock);
> +
>  	nvme_complete_cqes(nvmeq, start, end);
>  	return found;

And while we are at it:  I think for the irq-driven queuest in a
separate queue for poll setup we might not even need to take the
CQ lock.  Which might be an argument for only allowing polling
if we have the separate queues just to keep everything simple.

[PATCH 1/6] nvme: don't disable local ints for polled queue

[Max Gurtovoy]

On 11/10/2018 5:13 PM, Jens Axboe wrote:
> A polled queued doesn't trigger interrupts, so it's always safe
> to grab the queue lock without disabling interrupts.


Jens,

can you share the added value in performance for this change ?



> Cc: Keith Busch <keith.busch at intel.com>
> Cc: linux-nvme at lists.infradead.org
> Signed-off-by: Jens Axboe <axboe at kernel.dk>
> ---
>   drivers/nvme/host/pci.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6aa86dfcb32c..a6e3fbddfadf 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1061,15 +1061,26 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>   
>   static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
>   {
> +	unsigned long flags = 0; /* gcc 7.x fail */
>   	u16 start, end;
> -	bool found;
> +	bool found, has_irq;
>   
>   	if (!nvme_cqe_pending(nvmeq))
>   		return 0;
>   
> -	spin_lock_irq(&nvmeq->cq_lock);
> +	/*
> +	 * Polled queue doesn't have an IRQ, no need to disable ints
> +	 */
> +	has_irq = !nvmeq->polled;
> +	if (has_irq)
> +		local_irq_save(flags);
> +
> +	spin_lock(&nvmeq->cq_lock);
>   	found = nvme_process_cq(nvmeq, &start, &end, tag);
> -	spin_unlock_irq(&nvmeq->cq_lock);
> +	spin_unlock(&nvmeq->cq_lock);
> +
> +	if (has_irq)
> +		local_irq_restore(flags);
>   
>   	nvme_complete_cqes(nvmeq, start, end);
>   	return found;

[PATCH 1/6] nvme: don't disable local ints for polled queue

[Jens Axboe]

On 11/14/18 1:43 AM, Christoph Hellwig wrote:
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 6aa86dfcb32c..a6e3fbddfadf 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -1061,15 +1061,26 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>>  
>>  static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
>>  {
>> +	unsigned long flags = 0; /* gcc 7.x fail */
>>  	u16 start, end;
>> -	bool found;
>> +	bool found, has_irq;
>>  
>>  	if (!nvme_cqe_pending(nvmeq))
>>  		return 0;
>>  
>> -	spin_lock_irq(&nvmeq->cq_lock);
>> +	/*
>> +	 * Polled queue doesn't have an IRQ, no need to disable ints
>> +	 */
>> +	has_irq = !nvmeq->polled;
>> +	if (has_irq)
>> +		local_irq_save(flags);
>> +
>> +	spin_lock(&nvmeq->cq_lock);
>>  	found = nvme_process_cq(nvmeq, &start, &end, tag);
>> -	spin_unlock_irq(&nvmeq->cq_lock);
>> +	spin_unlock(&nvmeq->cq_lock);
>> +
>> +	if (has_irq)
>> +		local_irq_restore(flags);
> 
> Eww, this just looks ugly.  Given that we are micro-optimizing here
> I'd rather just use a different ->poll implementation and thus blk_mq_ops
> if we have a separate poll code.  Then we could leave the existing
> __nvme_poll as-is and have a new nvme_poll_noirq something like this:

Yeah good point, gets rid of those branches too. I'll make that change.

> And while we are at it:  I think for the irq-driven queuest in a
> separate queue for poll setup we might not even need to take the
> CQ lock.  Which might be an argument for only allowing polling
> if we have the separate queues just to keep everything simple.

We can definitely move in that direction, I'll take a look at what
is feasible.

-- 
Jens Axboe

[PATCH 1/6] nvme: don't disable local ints for polled queue

[Keith Busch]

On Wed, Nov 14, 2018@12:43:22AM -0800, Christoph Hellwig wrote:
> static int nvme_poll_noirq(struct blk_mq_hw_ctx *hctx, unsigned int tag)
> {
>         struct nvme_queue *nvmeq = hctx->driver_data;
> >  	u16 start, end;
> 	bool found;
> >  
> >  	if (!nvme_cqe_pending(nvmeq))
> >  		return 0;
> >  
> > +	spin_lock(&nvmeq->cq_lock);
> >  	found = nvme_process_cq(nvmeq, &start, &end, tag);
> > +	spin_unlock(&nvmeq->cq_lock);
> > +
> >  	nvme_complete_cqes(nvmeq, start, end);
> >  	return found;
> 
> And while we are at it:  I think for the irq-driven queues in a
> separate queue for poll setup we might not even need to take the
> CQ lock.  Which might be an argument for only allowing polling
> if we have the separate queues just to keep everything simple.

That's a pretty cool observation. We still poll interrupt driven queues
in the timeout path as a sanity check (it really has helped in debugging
timeout issues), but we can temporarily disable the cq's irq and be
lockless.