Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable

[Christoph Hellwig <hch@lst.de>]

On Wed, Mar 04, 2020 at 10:12:46AM -0800, Keith Busch wrote:
> The timeout handler can use the existing nvme_poll() if it needs to
> check a polled queue, allowing nvme_poll_irqdisable() to handle only
> irq driven queues for the remaining callers.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/pci.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 02f22c63adcf..227e2aed7e08 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1020,35 +1020,20 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>  }
>  
>  /*
> - * Poll for completions any queue, including those not dedicated to polling.
> + * Poll for completions for any interrupt driven queue
>   * Can be called from any context.
>   */
> -static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
> +static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
>  {
>  	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
>  
> +	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> +	nvme_process_cq(nvmeq);
> +	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));

This could use a:

	WARN_ON_ONCE(test_bit(NVMEQ_POLLED, &nvmeq->flags));

>  }
>  
> -static int nvme_poll(struct blk_mq_hw_ctx *hctx)
> +static int __nvme_poll(struct nvme_queue *nvmeq)

Do we really need the magic __nvme_poll?  struct request has a
pointer to the blk_mq_hw_ctx, so we could just pass that.

> +	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags))
> +		nvme_poll_irqdisable(nvmeq);
> +	else
> +		__nvme_poll(nvmeq);
> +

Any reason for the odd inversion and not simply:

	if (test_bit(NVMEQ_POLLED, &nvmeq->flags))
		nvme_poll(req->mq_hctx);
	else
		nvme_poll_irqdisable(nvmeq);

?

Otherwise looks good.

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