[PATCH 1/2] nvme-pci: fix freeing single sgl

[PATCH 1/2] nvme-pci: fix freeing single sgl

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There may only be a single DMA mapped entry from multiple physical
segments, which means we don't allocate a separte SGL list. Check the
number of allocations prior to know if we need to free something.

Freeing a single list allocation is the same for both PRP and SGL
usages, so we don't need to check the use_sgl flag anymore.

Fixes: 01df742d8c5c0 ("nvme-pci: remove SGL segment descriptors")
Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
Niklas,
This is a little different than the one you tested, so I didn't include
your "Tested-by". I'm confident this is patch still fixes the issue,
though.

 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 a331fbfa9a667..47d6b0023e3a8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -556,7 +556,7 @@ static void nvme_unmap_data(struct nvme_dev *dev, struct request *req)
 	if (iod->nr_allocations == 0)
 		dma_pool_free(dev->prp_small_pool, iod->list[0].sg_list,
 			      iod->first_dma);
-	else if (iod->use_sgl)
+	else if (iod->nr_allocations == 1)
 		dma_pool_free(dev->prp_page_pool, iod->list[0].sg_list,
 			      iod->first_dma);
 	else
-- 
2.30.2

[PATCH 2/2] nvme-pci: remove iod use_sgls

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

It's not used anywhere anymore, so remove it.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 47d6b0023e3a8..d68e2db00d0d0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -230,7 +230,6 @@ union nvme_descriptor {
 struct nvme_iod {
 	struct nvme_request req;
 	struct nvme_command cmd;
-	bool use_sgl;
 	bool aborted;
 	s8 nr_allocations;	/* PRP list pool allocations. 0 means small
 				   pool in use */
@@ -808,8 +807,7 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
 		goto out_free_sg;
 	}
 
-	iod->use_sgl = nvme_pci_use_sgls(dev, req, iod->sgt.nents);
-	if (iod->use_sgl)
+	if (nvme_pci_use_sgls(dev, req, iod->sgt.nents))
 		ret = nvme_pci_setup_sgls(dev, req, &cmnd->rw);
 	else
 		ret = nvme_pci_setup_prps(dev, req, &cmnd->rw);
-- 
2.30.2

Re: [PATCH 1/2] nvme-pci: fix freeing single sgl

[Christoph Hellwig]

Thanks,

I've applied both patches to nvme-6.3.

Re: [PATCH 1/2] nvme-pci: fix freeing single sgl

[Niklas Schnelle]

On Fri, 2023-02-10 at 10:03 -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> There may only be a single DMA mapped entry from multiple physical
> segments, which means we don't allocate a separte SGL list. Check the
> number of allocations prior to know if we need to free something.
> 
> Freeing a single list allocation is the same for both PRP and SGL
> usages, so we don't need to check the use_sgl flag anymore.
> 
> Fixes: 01df742d8c5c0 ("nvme-pci: remove SGL segment descriptors")
> Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> Niklas,
> This is a little different than the one you tested, so I didn't include
> your "Tested-by". I'm confident this is patch still fixes the issue,
> though.
> 
>  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 a331fbfa9a667..47d6b0023e3a8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -556,7 +556,7 @@ static void nvme_unmap_data(struct nvme_dev *dev, struct request *req)
>  	if (iod->nr_allocations == 0)
>  		dma_pool_free(dev->prp_small_pool, iod->list[0].sg_list,
>  			      iod->first_dma);
> -	else if (iod->use_sgl)
> +	else if (iod->nr_allocations == 1)
>  		dma_pool_free(dev->prp_page_pool, iod->list[0].sg_list,
>  			      iod->first_dma);
>  	else


Ah nice that makes it look cleaner and you got rid of the use_sgl flag.
I gave the two new patches a quick test on top of today's linux-next
and they work fine too. In fact while I don't have hard numbers and my
environment is quite noisy and there is of course other changes at play
too but linux-next does seem to get a bit more IOPS than v6.2-rc8 in my
FIO 4k random read test.

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>