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

[JeffleXu <jefflexu@linux.alibaba.com>]

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