[PATCH] nvme-pci: deduplicate empty request list checks in nvme_queue_rqs()

[PATCH] nvme-pci: deduplicate empty request list checks in nvme_queue_rqs()

[Caleb Sander Mateos]

nvme_queue_rqs() checks that nvmeq is non-NULL before calling
nvme_submit_cmds() and nvme_submit_cmds() checks that submit_list is
non-empty before doing anything. A NULL nvmeq means no requests were
processed from the rqlist in nvme_queue_rqs() since the last call to
nvme_submit_cmds(), which implies submit_list is empty. So just check
that submit_list is non-empty before calling nvme_submit_cmds() and drop
the check for NULL nvmeq.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/nvme/host/pci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 15c12e6cba88..8b562563ef89 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1199,13 +1199,10 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct rq_list *rqlist)
 {
 	struct request *req;
 
-	if (rq_list_empty(rqlist))
-		return;
-
 	spin_lock(&nvmeq->sq_lock);
 	while ((req = rq_list_pop(rqlist))) {
 		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
 
 		nvme_sq_copy_cmd(nvmeq, &iod->cmd);
@@ -1230,25 +1227,26 @@ static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
 
 static void nvme_queue_rqs(struct rq_list *rqlist)
 {
 	struct rq_list submit_list = { };
 	struct rq_list requeue_list = { };
-	struct nvme_queue *nvmeq = NULL;
+	struct nvme_queue *nvmeq;
 	struct request *req;
 
 	while ((req = rq_list_pop(rqlist))) {
-		if (nvmeq && nvmeq != req->mq_hctx->driver_data)
+		if (!rq_list_empty(&submit_list) &&
+		    nvmeq != req->mq_hctx->driver_data)
 			nvme_submit_cmds(nvmeq, &submit_list);
 		nvmeq = req->mq_hctx->driver_data;
 
 		if (nvme_prep_rq_batch(nvmeq, req))
 			rq_list_add_tail(&submit_list, req);
 		else
 			rq_list_add_tail(&requeue_list, req);
 	}
 
-	if (nvmeq)
+	if (!rq_list_empty(&submit_list))
 		nvme_submit_cmds(nvmeq, &submit_list);
 	*rqlist = requeue_list;
 }
 
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
-- 
2.45.2

Re: [PATCH] nvme-pci: deduplicate empty request list checks in nvme_queue_rqs()

[Christoph Hellwig]

On Tue, Nov 18, 2025 at 07:17:00PM -0700, Caleb Sander Mateos wrote:
> nvme_queue_rqs() checks that nvmeq is non-NULL before calling
> nvme_submit_cmds() and nvme_submit_cmds() checks that submit_list is
> non-empty before doing anything. A NULL nvmeq means no requests were
> processed from the rqlist in nvme_queue_rqs() since the last call to
> nvme_submit_cmds(), which implies submit_list is empty. So just check
> that submit_list is non-empty before calling nvme_submit_cmds() and drop
> the check for NULL nvmeq.

What is the rationale for this?  I had a hard time understanding the
logic in the new version, so I don't think this helps with readability
at least.

Re: [PATCH] nvme-pci: deduplicate empty request list checks in nvme_queue_rqs()

[Caleb Sander Mateos]

On Tue, Nov 18, 2025 at 9:48 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Nov 18, 2025 at 07:17:00PM -0700, Caleb Sander Mateos wrote:
> > nvme_queue_rqs() checks that nvmeq is non-NULL before calling
> > nvme_submit_cmds() and nvme_submit_cmds() checks that submit_list is
> > non-empty before doing anything. A NULL nvmeq means no requests were
> > processed from the rqlist in nvme_queue_rqs() since the last call to
> > nvme_submit_cmds(), which implies submit_list is empty. So just check
> > that submit_list is non-empty before calling nvme_submit_cmds() and drop
> > the check for NULL nvmeq.
>
> What is the rationale for this?  I had a hard time understanding the
> logic in the new version, so I don't think this helps with readability
> at least.

Just trying to avoid some branches. But if you prefer the current
version, that's fine.

Best,
Caleb

Re: [PATCH] nvme-pci: deduplicate empty request list checks in nvme_queue_rqs()

[Christoph Hellwig]

On Wed, Nov 19, 2025 at 08:01:30AM -0800, Caleb Sander Mateos wrote:
> On Tue, Nov 18, 2025 at 9:48 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Nov 18, 2025 at 07:17:00PM -0700, Caleb Sander Mateos wrote:
> > > nvme_queue_rqs() checks that nvmeq is non-NULL before calling
> > > nvme_submit_cmds() and nvme_submit_cmds() checks that submit_list is
> > > non-empty before doing anything. A NULL nvmeq means no requests were
> > > processed from the rqlist in nvme_queue_rqs() since the last call to
> > > nvme_submit_cmds(), which implies submit_list is empty. So just check
> > > that submit_list is non-empty before calling nvme_submit_cmds() and drop
> > > the check for NULL nvmeq.
> >
> > What is the rationale for this?  I had a hard time understanding the
> > logic in the new version, so I don't think this helps with readability
> > at least.
> 
> Just trying to avoid some branches. But if you prefer the current
> version, that's fine.

I find the current version easier to follow.  If there is a good
reason for the new variant I'm ok with it.