[PATCH 0/5] nvme-pci: iod optimisations

[PATCH 0/5] nvme-pci: iod optimisations

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The series adjusts the struct nvme_iod fields for optimal sizing. The
end result on 64-bit arch removes 24 bytes, from 152 down to 128. We
typically allocate many thousands of these, and the cache aligned size
is a nice bonus.

Keith Busch (5):
  nvme-pci: remove nvme_queue from nvme_iod
  nvme-pci: iod's 'aborted' is a bool
  nvme-pci: iod npages fits in s8
  nvme-pci: iod nents fits in s8
  nvme-pci: move iod dma_len to better pack

 drivers/nvme/host/pci.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

-- 
2.30.2

[PATCH 1/5] nvme-pci: remove nvme_queue from nvme_iod

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

We can get the nvme_queue from the req just as easily, so save some
space by removing the duplicate pointer to the same structure.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 644664098ae7..138d408f7306 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -226,7 +226,6 @@ struct nvme_queue {
 struct nvme_iod {
 	struct nvme_request req;
 	struct nvme_command cmd;
-	struct nvme_queue *nvmeq;
 	bool use_sgl;
 	int aborted;
 	int npages;		/* In the PRP list. 0 means small pool in use */
@@ -431,11 +430,6 @@ static int nvme_pci_init_request(struct blk_mq_tag_set *set,
 {
 	struct nvme_dev *dev = set->driver_data;
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	int queue_idx = (set == &dev->tagset) ? hctx_idx + 1 : 0;
-	struct nvme_queue *nvmeq = &dev->queues[queue_idx];
-
-	BUG_ON(!nvmeq);
-	iod->nvmeq = nvmeq;
 
 	nvme_req(req)->ctrl = &dev->ctrl;
 	nvme_req(req)->cmd = &iod->cmd;
@@ -529,7 +523,7 @@ static void **nvme_pci_iod_list(struct request *req)
 
 static inline bool nvme_pci_use_sgls(struct nvme_dev *dev, struct request *req)
 {
-	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
 	int nseg = blk_rq_nr_phys_segments(req);
 	unsigned int avg_seg_size;
 
@@ -537,7 +531,7 @@ static inline bool nvme_pci_use_sgls(struct nvme_dev *dev, struct request *req)
 
 	if (!nvme_ctrl_sgl_supported(&dev->ctrl))
 		return false;
-	if (!iod->nvmeq->qid)
+	if (!nvmeq->qid)
 		return false;
 	if (!sgl_threshold || avg_seg_size < sgl_threshold)
 		return false;
@@ -843,6 +837,7 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
 	int nr_mapped;
 
 	if (blk_rq_nr_phys_segments(req) == 1) {
+		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
 		struct bio_vec bv = req_bvec(req);
 
 		if (!is_pci_p2pdma_page(bv.bv_page)) {
@@ -850,7 +845,7 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
 				return nvme_setup_prp_simple(dev, req,
 							     &cmnd->rw, &bv);
 
-			if (iod->nvmeq->qid && sgl_threshold &&
+			if (nvmeq->qid && sgl_threshold &&
 			    nvme_ctrl_sgl_supported(&dev->ctrl))
 				return nvme_setup_sgl_simple(dev, req,
 							     &cmnd->rw, &bv);
@@ -1030,12 +1025,16 @@ static void nvme_queue_rqs(struct request **rqlist)
 
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
-	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	struct nvme_dev *dev = iod->nvmeq->dev;
+	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
+	struct nvme_dev *dev = nvmeq->dev;
+
+	if (blk_integrity_rq(req)) {
+	        struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
 
-	if (blk_integrity_rq(req))
 		dma_unmap_page(dev->dev, iod->meta_dma,
 			       rq_integrity_vec(req)->bv_len, rq_data_dir(req));
+	}
+
 	if (blk_rq_nr_phys_segments(req))
 		nvme_unmap_data(dev, req);
 }
@@ -1283,8 +1282,7 @@ static int adapter_delete_sq(struct nvme_dev *dev, u16 sqid)
 
 static void abort_endio(struct request *req, blk_status_t error)
 {
-	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	struct nvme_queue *nvmeq = iod->nvmeq;
+	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
 
 	dev_warn(nvmeq->dev->ctrl.device,
 		 "Abort status: 0x%x", nvme_req(req)->status);
@@ -1346,7 +1344,7 @@ static void nvme_warn_reset(struct nvme_dev *dev, u32 csts)
 static enum blk_eh_timer_return nvme_timeout(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	struct nvme_queue *nvmeq = iod->nvmeq;
+	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
 	struct nvme_dev *dev = nvmeq->dev;
 	struct request *abort_req;
 	struct nvme_command cmd = { };
-- 
2.30.2

[PATCH 2/5] nvme-pci: iod's 'aborted' is a bool

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

It's only true or false, so make this a bool to reflect that and save
some space in nvme_iod.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 138d408f7306..a8ae043e33de 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -227,7 +227,7 @@ struct nvme_iod {
 	struct nvme_request req;
 	struct nvme_command cmd;
 	bool use_sgl;
-	int aborted;
+	bool aborted;
 	int npages;		/* In the PRP list. 0 means small pool in use */
 	int nents;		/* Used in scatterlist */
 	dma_addr_t first_dma;
@@ -904,7 +904,7 @@ static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
 	blk_status_t ret;
 
-	iod->aborted = 0;
+	iod->aborted = false;
 	iod->npages = -1;
 	iod->nents = 0;
 
@@ -1425,7 +1425,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
 		atomic_inc(&dev->ctrl.abort_limit);
 		return BLK_EH_RESET_TIMER;
 	}
-	iod->aborted = 1;
+	iod->aborted = true;
 
 	cmd.abort.opcode = nvme_admin_abort_cmd;
 	cmd.abort.cid = nvme_cid(req);
-- 
2.30.2

[PATCH 3/5] nvme-pci: iod npages fits in s8

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The largest allowed transfer is 4MB, which can use at most 1025 PRPs.
Each PRP is 8 bytes, so the maximum number of 4k nvme pages needed for
the iod_list is 3, which fits in an 's8'.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 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 a8ae043e33de..9e5bbf4e3e07 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -228,7 +228,7 @@ struct nvme_iod {
 	struct nvme_command cmd;
 	bool use_sgl;
 	bool aborted;
-	int npages;		/* In the PRP list. 0 means small pool in use */
+	s8 npages;		/* In the PRP list. 0 means small pool in use */
 	int nents;		/* Used in scatterlist */
 	dma_addr_t first_dma;
 	unsigned int dma_len;	/* length of single DMA segment mapping */
-- 
2.30.2

[PATCH 4/5] nvme-pci: iod nents fits in s8

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The maximum number of 'nents' allowed by the queue limit is 127, which
fits in an 's8'.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 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 9e5bbf4e3e07..546de3c2000b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -229,7 +229,7 @@ struct nvme_iod {
 	bool use_sgl;
 	bool aborted;
 	s8 npages;		/* In the PRP list. 0 means small pool in use */
-	int nents;		/* Used in scatterlist */
+	s8 nents;		/* Used in scatterlist */
 	dma_addr_t first_dma;
 	unsigned int dma_len;	/* length of single DMA segment mapping */
 	dma_addr_t meta_dma;
-- 
2.30.2

[PATCH 5/5] nvme-pci: move iod dma_len to better pack

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The 32-bit 'dma_len' field packs in the iod struct above the dma_addr_t
without creating a gap.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 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 546de3c2000b..66f7204ab905 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -230,8 +230,8 @@ struct nvme_iod {
 	bool aborted;
 	s8 npages;		/* In the PRP list. 0 means small pool in use */
 	s8 nents;		/* Used in scatterlist */
-	dma_addr_t first_dma;
 	unsigned int dma_len;	/* length of single DMA segment mapping */
+	dma_addr_t first_dma;
 	dma_addr_t meta_dma;
 	struct scatterlist *sg;
 };
-- 
2.30.2

Re: [PATCH 2/5] nvme-pci: iod's 'aborted' is a bool

[Chaitanya Kulkarni]

On 7/28/22 15:11, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> It's only true or false, so make this a bool to reflect that and save
> some space in nvme_iod.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 3/5] nvme-pci: iod npages fits in s8

[Chaitanya Kulkarni]

On 7/28/22 15:11, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The largest allowed transfer is 4MB, which can use at most 1025 PRPs.
> Each PRP is 8 bytes, so the maximum number of 4k nvme pages needed for
> the iod_list is 3, which fits in an 's8'.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 4/5] nvme-pci: iod nents fits in s8

[Chaitanya Kulkarni]

On 7/28/22 15:11, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The maximum number of 'nents' allowed by the queue limit is 127, which
> fits in an 's8'.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 5/5] nvme-pci: move iod dma_len to better pack

[Chaitanya Kulkarni]

On 7/28/22 15:11, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The 32-bit 'dma_len' field packs in the iod struct above the dma_addr_t
> without creating a gap.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 3/5] nvme-pci: iod npages fits in s8

[Christoph Hellwig]

On Thu, Jul 28, 2022 at 03:11:49PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The largest allowed transfer is 4MB, which can use at most 1025 PRPs.
> Each PRP is 8 bytes, so the maximum number of 4k nvme pages needed for
> the iod_list is 3, which fits in an 's8'.

Can we use the chance to rename the field to nr_allocations or
so?  The nr_pages always confused me, and the comment does not exactly
help.

Re: [PATCH 4/5] nvme-pci: iod nents fits in s8

[Christoph Hellwig]

On Thu, Jul 28, 2022 at 03:11:50PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The maximum number of 'nents' allowed by the queue limit is 127, which
> fits in an 's8'.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  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 9e5bbf4e3e07..546de3c2000b 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -229,7 +229,7 @@ struct nvme_iod {
>  	bool use_sgl;
>  	bool aborted;
>  	s8 npages;		/* In the PRP list. 0 means small pool in use */
> -	int nents;		/* Used in scatterlist */
> +	s8 nents;		/* Used in scatterlist */

Can we please have a BUILD_BUG_ON to protect us from an increased
max_segments?  As I can totally see us incrementing that at some point,
and the limit is somewhat close.

Re: [PATCH 4/5] nvme-pci: iod nents fits in s8

[Keith Busch]

On Fri, Jul 29, 2022 at 03:21:21PM +0200, Christoph Hellwig wrote:
> On Thu, Jul 28, 2022 at 03:11:50PM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The maximum number of 'nents' allowed by the queue limit is 127, which
> > fits in an 's8'.
> > 
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >  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 9e5bbf4e3e07..546de3c2000b 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -229,7 +229,7 @@ struct nvme_iod {
> >  	bool use_sgl;
> >  	bool aborted;
> >  	s8 npages;		/* In the PRP list. 0 means small pool in use */
> > -	int nents;		/* Used in scatterlist */
> > +	s8 nents;		/* Used in scatterlist */
> 
> Can we please have a BUILD_BUG_ON to protect us from an increased
> max_segments?  As I can totally see us incrementing that at some point,
> and the limit is somewhat close.

Yes, will do. Also, this one is only ever assigned unsigned values, so I'll
make this a u8 in the next version.