[RFC] nvme/pci: allocate separate interrupt for reserved non-polled IO queue

[RFC] nvme/pci: allocate separate interrupt for reserved non-polled IO queue

[Jeffle Xu]

One queue will be reserved for non-polled IO when nvme.poll_queues is
greater or equal than the number of IO queues that the nvme controller
can provide. Currently the reserved queue for non-polled IO will reuse
the interrupt used by admin queue in this case, e.g, vector 0.

This can work and the performance may not be an issue since the admin
queue is used unfrequently. However this behaviour may be inconsistent
with that when nvme.poll_queues is smaller than the number of IO
queues available.

Thus allocate separate interrupt for this reserved queue, and thus make
the behaviour consistent.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 drivers/nvme/host/pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 899d2f4d7ab6..ff3e28a8a9e0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2042,12 +2042,14 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 
 	/*
 	 * Poll queues don't need interrupts, but we need at least one IO
-	 * queue left over for non-polled IO.
+	 * queue left over for non-polled IO. Also one interrupt for admin
+	 * queue.
 	 */
 	this_p_queues = dev->nr_poll_queues;
 	if (this_p_queues >= nr_io_queues) {
 		this_p_queues = nr_io_queues - 1;
-		irq_queues = 1;
+		/* Allocate separate interrupt for reserved non-polled IO queue. */
+		irq_queues = 2;
 	} else {
 		irq_queues = nr_io_queues - this_p_queues + 1;
 	}
-- 
2.27.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC] nvme/pci: allocate separate interrupt for reserved non-polled IO queue

[JeffleXu]

Any comments?

On 9/22/20 12:28 PM, Jeffle Xu wrote:
> One queue will be reserved for non-polled IO when nvme.poll_queues is
> greater or equal than the number of IO queues that the nvme controller
> can provide. Currently the reserved queue for non-polled IO will reuse
> the interrupt used by admin queue in this case, e.g, vector 0.
>
> This can work and the performance may not be an issue since the admin
> queue is used unfrequently. However this behaviour may be inconsistent
> with that when nvme.poll_queues is smaller than the number of IO
> queues available.
>
> Thus allocate separate interrupt for this reserved queue, and thus make
> the behaviour consistent.
>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>   drivers/nvme/host/pci.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 899d2f4d7ab6..ff3e28a8a9e0 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2042,12 +2042,14 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
>   
>   	/*
>   	 * Poll queues don't need interrupts, but we need at least one IO
> -	 * queue left over for non-polled IO.
> +	 * queue left over for non-polled IO. Also one interrupt for admin
> +	 * queue.
>   	 */
>   	this_p_queues = dev->nr_poll_queues;
>   	if (this_p_queues >= nr_io_queues) {
>   		this_p_queues = nr_io_queues - 1;
> -		irq_queues = 1;
> +		/* Allocate separate interrupt for reserved non-polled IO queue. */
> +		irq_queues = 2;
>   	} else {
>   		irq_queues = nr_io_queues - this_p_queues + 1;
>   	}

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC] nvme/pci: allocate separate interrupt for reserved non-polled IO queue

[Christoph Hellwig]

On Tue, Sep 22, 2020 at 12:28:16PM +0800, Jeffle Xu wrote:
> One queue will be reserved for non-polled IO when nvme.poll_queues is
> greater or equal than the number of IO queues that the nvme controller
> can provide. Currently the reserved queue for non-polled IO will reuse
> the interrupt used by admin queue in this case, e.g, vector 0.
> 
> This can work and the performance may not be an issue since the admin
> queue is used unfrequently. However this behaviour may be inconsistent
> with that when nvme.poll_queues is smaller than the number of IO
> queues available.
> 
> Thus allocate separate interrupt for this reserved queue, and thus make
> the behaviour consistent.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>

This code looks good, but the function already is a mess without the
addition.  What do you think about this variant?


diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 899d2f4d7ab612..43055138d59a47 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2038,31 +2038,29 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
 		.calc_sets	= nvme_calc_irq_sets,
 		.priv		= dev,
 	};
-	unsigned int irq_queues, this_p_queues;
+	unsigned int irq_queues, poll_queues;
 
 	/*
-	 * Poll queues don't need interrupts, but we need at least one IO
-	 * queue left over for non-polled IO.
+	 * Poll queues don't need interrupts, but we need at least one I/O queue
+	 * left over for non-polled I/O.
 	 */
-	this_p_queues = dev->nr_poll_queues;
-	if (this_p_queues >= nr_io_queues) {
-		this_p_queues = nr_io_queues - 1;
-		irq_queues = 1;
-	} else {
-		irq_queues = nr_io_queues - this_p_queues + 1;
-	}
-	dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
+	poll_queues = min(dev->nr_poll_queues, nr_io_queues - 1);
+	dev->io_queues[HCTX_TYPE_POLL] = poll_queues;
 
-	/* Initialize for the single interrupt case */
+	/*
+	 * Initialize for the single interrupt case, will be updated in
+	 * nvme_calc_irq_sets().
+	 */
 	dev->io_queues[HCTX_TYPE_DEFAULT] = 1;
 	dev->io_queues[HCTX_TYPE_READ] = 0;
 
 	/*
-	 * Some Apple controllers require all queues to use the
-	 * first vector.
+	 * Some Apple controllers require all queues to use the first vector.
 	 */
 	if (dev->ctrl.quirks & NVME_QUIRK_SINGLE_VECTOR)
 		irq_queues = 1;
+	else
+		irq_queues = 1 + (nr_io_queues - poll_queues);
 
 	return pci_alloc_irq_vectors_affinity(pdev, 1, irq_queues,
 			      PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC] nvme/pci: allocate separate interrupt for reserved non-polled IO queue

[JeffleXu]

Thanks for replying.


On 9/24/20 3:04 PM, Christoph Hellwig wrote:
> On Tue, Sep 22, 2020 at 12:28:16PM +0800, Jeffle Xu wrote:
>> One queue will be reserved for non-polled IO when nvme.poll_queues is
>> greater or equal than the number of IO queues that the nvme controller
>> can provide. Currently the reserved queue for non-polled IO will reuse
>> the interrupt used by admin queue in this case, e.g, vector 0.
>>
>> This can work and the performance may not be an issue since the admin
>> queue is used unfrequently. However this behaviour may be inconsistent
>> with that when nvme.poll_queues is smaller than the number of IO
>> queues available.
>>
>> Thus allocate separate interrupt for this reserved queue, and thus make
>> the behaviour consistent.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> This code looks good, but the function already is a mess without the
> addition.  What do you think about this variant?

Looks good to me. If you don't mind, I'd like to send a v2 patch 
containing your refactored code.

Also some trivial advice below.

>
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 899d2f4d7ab612..43055138d59a47 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2038,31 +2038,29 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
>   		.calc_sets	= nvme_calc_irq_sets,
>   		.priv		= dev,
>   	};
> -	unsigned int irq_queues, this_p_queues;
> +	unsigned int irq_queues, poll_queues;
>   
>   	/*
> -	 * Poll queues don't need interrupts, but we need at least one IO
> -	 * queue left over for non-polled IO.
> +	 * Poll queues don't need interrupts, but we need at least one I/O queue
> +	 * left over for non-polled I/O.
>   	 */
> -	this_p_queues = dev->nr_poll_queues;
> -	if (this_p_queues >= nr_io_queues) {
> -		this_p_queues = nr_io_queues - 1;
> -		irq_queues = 1;
> -	} else {
> -		irq_queues = nr_io_queues - this_p_queues + 1;
> -	}
> -	dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
> +	poll_queues = min(dev->nr_poll_queues, nr_io_queues - 1);
> +	dev->io_queues[HCTX_TYPE_POLL] = poll_queues;
>   
> -	/* Initialize for the single interrupt case */
> +	/*
> +	 * Initialize for the single interrupt case, will be updated in
> +	 * nvme_calc_irq_sets().
> +	 */
>   	dev->io_queues[HCTX_TYPE_DEFAULT] = 1;
>   	dev->io_queues[HCTX_TYPE_READ] = 0;
>   
>   	/*
> -	 * Some Apple controllers require all queues to use the
> -	 * first vector.
> +	 * Some Apple controllers require all queues to use the first vector.
>   	 */
>   	if (dev->ctrl.quirks & NVME_QUIRK_SINGLE_VECTOR)
>   		irq_queues = 1;
> +	else

> +		irq_queues = 1 + (nr_io_queues - poll_queues);

Personally it would be better if there's a comment to explicitly point 
out that there's one interrupt for admin queue.

It would be more friendly to the newborns.

such as

         /*
-        * Some Apple controllers require all queues to use the
-        * first vector.
+        * Some Apple controllers require all queues to use the first 
vector.
          */
         if (dev->ctrl.quirks & NVME_QUIRK_SINGLE_VECTOR)
                 irq_queues = 1;

+       else {
+               /* One interrupt for admin queue.*/
+               irq_queues = 1 + (nr_io_queues - poll_queues);
+       }


>   
>   	return pci_alloc_irq_vectors_affinity(pdev, 1, irq_queues,
>   			      PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);



>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme