[PATCH v2] nvme: change the return type of nvme_poll()

[PATCH v2] nvme: change the return type of nvme_poll()

[Yongsoo Joo]

Change the return type of nvme_poll() to bool to reflect its boolean
return value.

Signed-off-by: Yongsoo Joo <ysjoo@kookmin.ac.kr>
---
Changes from v1 to v2:
- Changed the return type of nvme_poll() from int to bool, rather than
  modifying the internal return variable from bool to int (Christoph
  Hellwig).
- Updated the patch subject by replacing "fix" with "change" (Keith 
  Busch).

 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1a5ba80f1811..34650c03c120 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1206,7 +1206,7 @@ static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 }
 
-static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
+static bool nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 {
 	struct nvme_queue *nvmeq = hctx->driver_data;
 	bool found;
-- 
2.34.1


-- 

Re: [PATCH v2] nvme: change the return type of nvme_poll()

[Keith Busch]

On Mon, Dec 09, 2024 at 01:33:44PM +0000, Yongsoo Joo wrote:
> -static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> +static bool nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
>  {
>  	struct nvme_queue *nvmeq = hctx->driver_data;
>  	bool found;
> -- 

This function is registered with blk_mq_ops, so you would have to change
the prototype there too, and every other driver that also registers a
poll op. Honestly, it may not be worth the effort.

Re: [PATCH v2] nvme: change the return type of nvme_poll()

[Yongsoo Joo]

On Mon, Dec 09, 2024 at 08:44:28AM -0700, Keith Busch wrote:
> On Mon, Dec 09, 2024 at 01:33:44PM +0000, Yongsoo Joo wrote:
> > -static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> > +static bool nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> >  {
> >  	struct nvme_queue *nvmeq = hctx->driver_data;
> >  	bool found;
> > -- 
> 
> This function is registered with blk_mq_ops, so you would have to change
> the prototype there too, and every other driver that also registers a
> poll op. Honestly, it may not be worth the effort.
Oh, I overlooked the issue you pointed out, which brings me back to my initial 
suggestion. The function nvme_poll_cq() returns the number of found CQEs, but 
nvme_poll() seems to unnecessarily reduce this information to a boolean.
A minimal change from “bool found” to “int found” in nvme_poll() would 
preserve this information, allowing any interested researcher to make use of 
it. Also, the description of poll in blk_mq_ops can be expanded as follows:
	/**
	 * @poll: Called to poll for completion of a specific tag.
         * Returns the number of CQEs found.
	 */
Thank you for your time, and I look forward to hearing your opinion.

-- 

Re: [PATCH v2] nvme: change the return type of nvme_poll()

[Keith Busch]

On Mon, Dec 09, 2024 at 09:39:42PM +0000, Yongsoo Joo wrote:
> On Mon, Dec 09, 2024 at 08:44:28AM -0700, Keith Busch wrote:
> > On Mon, Dec 09, 2024 at 01:33:44PM +0000, Yongsoo Joo wrote:
> > > -static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> > > +static bool nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> > >  {
> > >  	struct nvme_queue *nvmeq = hctx->driver_data;
> > >  	bool found;
> > > -- 
> > 
> > This function is registered with blk_mq_ops, so you would have to change
> > the prototype there too, and every other driver that also registers a
> > poll op. Honestly, it may not be worth the effort.
> Oh, I overlooked the issue you pointed out, which brings me back to my initial 
> suggestion. The function nvme_poll_cq() returns the number of found CQEs, but 
> nvme_poll() seems to unnecessarily reduce this information to a boolean.

Can we just change nvme_poll_cq() instead to return a bool instead?

---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4c644bb7f0692..54910f8eadff8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1148,13 +1148,13 @@ static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 	}
 }
 
-static inline int nvme_poll_cq(struct nvme_queue *nvmeq,
-			       struct io_comp_batch *iob)
+static inline bool nvme_poll_cq(struct nvme_queue *nvmeq,
+				struct io_comp_batch *iob)
 {
-	int found = 0;
+	bool found = false;
 
 	while (nvme_cqe_pending(nvmeq)) {
-		found++;
+		found = true
 		/*
 		 * load-load control dependency between phase and the rest of
 		 * the cqe requires a full read memory barrier
--

> A minimal change from "bool found" to "int found" in nvme_poll() would 
> preserve this information, allowing any interested researcher to make use of 
> it. Also, the description of poll in blk_mq_ops can be expanded as follows:
>        /**
>         * @poll: Called to poll for completion of a specific tag.
>         * Returns the number of CQEs found.
>         */

Since you pointed this comment out, I see it is from an earlier versions
of this interface and outdated. The current behavior polls for any
completion rather than a specific tag.

Re: [PATCH v2] nvme: change the return type of nvme_poll()

[Yongsoo Joo]

On Mon, Dec 09, 2024 at 05:01:06PM -0700, Keith Busch wrote:
> On Mon, Dec 09, 2024 at 09:39:42PM +0000, Yongsoo Joo wrote:
> > On Mon, Dec 09, 2024 at 08:44:28AM -0700, Keith Busch wrote:
> > > On Mon, Dec 09, 2024 at 01:33:44PM +0000, Yongsoo Joo wrote:
> > > > -static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> > > > +static bool nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
> > > >  {
> > > >  	struct nvme_queue *nvmeq = hctx->driver_data;
> > > >  	bool found;
> > > > -- 
> > > 
> > > This function is registered with blk_mq_ops, so you would have to change
> > > the prototype there too, and every other driver that also registers a
> > > poll op. Honestly, it may not be worth the effort.
> > Oh, I overlooked the issue you pointed out, which brings me back to my initial 
> > suggestion. The function nvme_poll_cq() returns the number of found CQEs, but 
> > nvme_poll() seems to unnecessarily reduce this information to a boolean.
> 
> Can we just change nvme_poll_cq() instead to return a bool instead?

Thank you for your suggestion; it makes sense to me. We could consider modifying 
nvme_poll() to return the number of CQEs in the future when specific use cases 
arise. 
> 
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 4c644bb7f0692..54910f8eadff8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1148,13 +1148,13 @@ static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
>  	}
>  }
>  
> -static inline int nvme_poll_cq(struct nvme_queue *nvmeq,
> -			       struct io_comp_batch *iob)
> +static inline bool nvme_poll_cq(struct nvme_queue *nvmeq,
> +				struct io_comp_batch *iob)
>  {
> -	int found = 0;
> +	bool found = false;
>  
>  	while (nvme_cqe_pending(nvmeq)) {
> -		found++;
> +		found = true

  +             found = true;

>  		/*
>  		 * load-load control dependency between phase and the rest of
>  		 * the cqe requires a full read memory barrier
> --
> 
> > A minimal change from "bool found" to "int found" in nvme_poll() would 
> > preserve this information, allowing any interested researcher to make use of 
> > it. Also, the description of poll in blk_mq_ops can be expanded as follows:
> >        /**
> >         * @poll: Called to poll for completion of a specific tag.
            * @poll: Called to poll for completion of any CQE.
> >         * Returns the number of CQEs found.
> >         */
> 
> Since you pointed this comment out, I see it is from an earlier versions
> of this interface and outdated. The current behavior polls for any
> completion rather than a specific tag.
Oh, I see. How about revising the comment to "Called to poll for the 
completion of any CQE"? Do you think this change should be submitted as a 
seperated patch?


--