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

[Yongsoo Joo <ysjoo@kookmin.ac.kr>]

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?


--