[PATCH 0/4] nvme: fix iod size calculation in nvme_probe()

[PATCH 0/4] nvme: fix iod size calculation in nvme_probe()

[Chaitanya Kulkarni]

Hi,

This is a small patch series which fixes the IO size calulation in the
nvme_probe. First two patches are prep patches needed to get the right
value so that we can avoid oops in nvme_npages.

The third patch calculates the maximum value based on NVMe PRP and SGL
size.

The last patch has a fix for checkpatch reported warnings which were
present in the existing code prior to this patch-series.

I've tested this patch with different block sizes 4k-128k on NVMe QEMU
and NVMe PCIe (non-SGL) controller.

Regards,
Chaitanya

Chaitanya Kulkarni (4):
  nvme-core: use macro for ctrl page size default value
  nvme-pci: don't use ctrl page size value in probe
  nvme-pci: use max of PRP or SGL for iod size
  nvme-core: fix checkpatch warnings

 drivers/nvme/host/core.c | 15 +++++----------
 drivers/nvme/host/nvme.h |  7 +++++++
 drivers/nvme/host/pci.c  | 24 +++++++++++++++---------
 3 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/4] nvme-core: use macro for ctrl page size default value

[Chaitanya Kulkarni]

This is a preparation patch which is needed to centralize the page shift
value for the have ctrl->page_size.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 15 +++++----------
 drivers/nvme/host/nvme.h |  7 +++++++
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 898885630ad8..e0b47e77cbca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2344,12 +2344,7 @@ EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 {
-	/*
-	 * Default to a 4K page size, with the intention to update this
-	 * path in the future to accomodate architectures with differing
-	 * kernel and IO page sizes.
-	 */
-	unsigned dev_page_min, page_shift = 12;
+	unsigned dev_page_min;
 	int ret;
 
 	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
@@ -2359,20 +2354,20 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	}
 	dev_page_min = NVME_CAP_MPSMIN(ctrl->cap) + 12;
 
-	if (page_shift < dev_page_min) {
+	if (NVME_CTRL_PAGE_SHIFT < dev_page_min) {
 		dev_err(ctrl->device,
 			"Minimum device page size %u too large for host (%u)\n",
-			1 << dev_page_min, 1 << page_shift);
+			1 << dev_page_min, 1 << NVME_CTRL_PAGE_SHIFT);
 		return -ENODEV;
 	}
 
-	ctrl->page_size = 1 << page_shift;
+	ctrl->page_size = 1 << NVME_CTRL_PAGE_SHIFT;
 
 	if (NVME_CAP_CSS(ctrl->cap) & NVME_CAP_CSS_CSI)
 		ctrl->ctrl_config = NVME_CC_CSS_CSI;
 	else
 		ctrl->ctrl_config = NVME_CC_CSS_NVM;
-	ctrl->ctrl_config |= (page_shift - 12) << NVME_CC_MPS_SHIFT;
+	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
 	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
 	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
 	ctrl->ctrl_config |= NVME_CC_ENABLE;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 85d76981b66e..4a56897e9081 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -37,6 +37,13 @@ extern unsigned int admin_timeout;
 #define  NVME_INLINE_METADATA_SG_CNT  1
 #endif
 
+/*
+ * Default to a 4K page size, with the intention to update this
+ * path in the future to accomodate architectures with differing
+ * kernel and IO page sizes.
+ */
+#define NVME_CTRL_PAGE_SHIFT	12
+
 extern struct workqueue_struct *nvme_wq;
 extern struct workqueue_struct *nvme_reset_wq;
 extern struct workqueue_struct *nvme_delete_wq;
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-core: use macro for ctrl page size default value

[Keith Busch]

On Tue, Jul 07, 2020 at 05:58:28PM -0700, Chaitanya Kulkarni wrote:
> This is a preparation patch which is needed to centralize the page shift
> value for the have ctrl->page_size.

At this point I would recommend removing 'page_size' from 'struct ctrl'
and use a #define NVME_PAGE_SIZE in its place. There hasn't been any
attempt to work with different page sizes so I don't see much reason
for keeping the older code to support it. As an added bonus, a constant
simplifies several calculations in the io path.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 898885630ad8..e0b47e77cbca 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2344,12 +2344,7 @@ EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
>  
>  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  {
> -	/*
> -	 * Default to a 4K page size, with the intention to update this
> -	 * path in the future to accomodate architectures with differing
> -	 * kernel and IO page sizes.
> -	 */
> -	unsigned dev_page_min, page_shift = 12;
> +	unsigned dev_page_min;
>  	int ret;
>  
>  	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
> @@ -2359,20 +2354,20 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  	}
>  	dev_page_min = NVME_CAP_MPSMIN(ctrl->cap) + 12;
>  
> -	if (page_shift < dev_page_min) {
> +	if (NVME_CTRL_PAGE_SHIFT < dev_page_min) {
>  		dev_err(ctrl->device,
>  			"Minimum device page size %u too large for host (%u)\n",
> -			1 << dev_page_min, 1 << page_shift);
> +			1 << dev_page_min, 1 << NVME_CTRL_PAGE_SHIFT);
>  		return -ENODEV;
>  	}
>  
> -	ctrl->page_size = 1 << page_shift;
> +	ctrl->page_size = 1 << NVME_CTRL_PAGE_SHIFT;
>  
>  	if (NVME_CAP_CSS(ctrl->cap) & NVME_CAP_CSS_CSI)
>  		ctrl->ctrl_config = NVME_CC_CSS_CSI;
>  	else
>  		ctrl->ctrl_config = NVME_CC_CSS_NVM;
> -	ctrl->ctrl_config |= (page_shift - 12) << NVME_CC_MPS_SHIFT;
> +	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
>  	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
>  	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
>  	ctrl->ctrl_config |= NVME_CC_ENABLE;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 85d76981b66e..4a56897e9081 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -37,6 +37,13 @@ extern unsigned int admin_timeout;
>  #define  NVME_INLINE_METADATA_SG_CNT  1
>  #endif
>  
> +/*
> + * Default to a 4K page size, with the intention to update this
> + * path in the future to accomodate architectures with differing
> + * kernel and IO page sizes.
> + */
> +#define NVME_CTRL_PAGE_SHIFT	12
> +
>  extern struct workqueue_struct *nvme_wq;
>  extern struct workqueue_struct *nvme_reset_wq;
>  extern struct workqueue_struct *nvme_delete_wq;

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-core: use macro for ctrl page size default value

[Chaitanya Kulkarni]

On 7/8/20 15:23, Keith Busch wrote:
> On Tue, Jul 07, 2020 at 05:58:28PM -0700, Chaitanya Kulkarni wrote:
>> This is a preparation patch which is needed to centralize the page shift
>> value for the have ctrl->page_size.
> At this point I would recommend removing 'page_size' from 'struct ctrl'
> and use a #define NVME_PAGE_SIZE in its place. There hasn't been any
> attempt to work with different page sizes so I don't see much reason
> for keeping the older code to support it. As an added bonus, a constant
> simplifies several calculations in the io path.
> 
Sounds good. I'll send a V2.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/4] nvme-pci: don't use ctrl page size value in probe

[Chaitanya Kulkarni]

In nvme_probe() when calculating mempool size nvme_npages uses
dev->ctrl.page_size which is initialized in the following code path:-

Ctx insmod :
nvme_probe()
 nvme_reset_ctrl()
  queue_work(nvme_reset_work()

Ctx Workqueue :
nvme_reset_work()
 nvme_pci_configure_admin_queue()
  nvme_enable_ctrl()
   ctrl->page_size = 1 << NVME_CTRL_PAGE_SHIFT.

When nvme_pci_iod_alloc_size() is called with false as last argument it
results in following oops since dev->ctrl.page_size used before we set
it in the nvme_enable_ctrl() in above path :-

Entering kdb (current=0xffff88881fc0c980, pid 339) on processor 0 Oops: (null)
due to oops @ 0xffffffffa05f1723
CPU: 0 PID: 339 Comm: kworker/0:2 Tainted: G        W  OE     5.8.0-rc1nvme-5.9+ #20
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.4
Workqueue: events work_for_cpu_fn
RIP: 0010:nvme_probe+0x263/0x502 [nvme]
Code: 00 00 66 41 81 7c 24 3c 4d 14 0f 84 98 01 00 00 3d 0f 1e 01 00 0f 84 aa 01 00 00 8b 8b a0 0c 00 2
RSP: 0018:ffffc90000d9bdd8 EFLAGS: 00010246
RAX: 0000000000000fff RBX: ffff8887da128000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000246
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: ffff8887c29f5570 R12: ffff888813c37000
R13: 0000000000000202 R14: 0000000fffffffe0 R15: ffff888813c370b0
FS:  0000000000000000(0000) GS:ffff88880fe00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f23185131a0 CR3: 0000000811c54000 CR4: 00000000003406f0
Call Trace:
 local_pci_probe+0x42/0x80
 work_for_cpu_fn+0x16/0x20
 process_one_work+0x24e/0x5a0
 ? __schedule+0x353/0x840
 worker_thread+0x1d5/0x380
 ? process_one_work+0x5a0/0x5a0
 kthread+0x135/0x150
 ? kthread_create_on_node+0x60/0x60
 ret_from_fork+0x22/0x30

This patch uses local variable instead of dev->ctrl.page_size with the
same value which we set in the nvme_enable_ctrl().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1ac0717c04d3..cef87ad2323f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -348,8 +348,15 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
  */
 static int nvme_npages(unsigned size, struct nvme_dev *dev)
 {
-	unsigned nprps = DIV_ROUND_UP(size + dev->ctrl.page_size,
-				      dev->ctrl.page_size);
+	/*
+	 * At this point ctrl->page_size is not initialized. Use same the
+	 * values present in nvme_enable_ctrl() ctrl->page_size. Any change in
+	 * the value of ctrl->page_size in nvme_enable_ctrl() should reflect
+	 * here.
+	 */
+	unsigned int page_size = 1 << NVME_CTRL_PAGE_SHIFT;
+	unsigned nprps = DIV_ROUND_UP(size + page_size, page_size);
+
 	return DIV_ROUND_UP(8 * nprps, PAGE_SIZE - 8);
 }
 
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/4] nvme-pci: use max of PRP or SGL for iod size

[Chaitanya Kulkarni]

From the initial implementation of NVMe SGL kernel support
commit a7a7cbe353a5 ("nvme-pci: add SGL support") with addition of the
commit 943e942e6266 ("nvme-pci: limit max IO size and segments to avoid
high order allocations") now there is only caller left for
nvme_pci_iod_alloc_size() which statically passes true for last
parameter that calculates allocation size based on SGL since we need
size of biggest command supported for mempool allocation.

This patch modifies the helper functions nvme_pci_iod_alloc_size() such
that it is now uses maximum of PRP and SGL size for iod allocation size
calculation.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/pci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cef87ad2323f..0588bd053a2a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -370,14 +370,13 @@ static int nvme_pci_npages_sgl(unsigned int num_seg)
 }
 
 static size_t nvme_pci_iod_alloc_size(struct nvme_dev *dev,
-		unsigned int size, unsigned int nseg, bool use_sgl)
+		unsigned int size, unsigned int nseg)
 {
-	size_t alloc_size;
+	size_t npages_sgl = nvme_pci_npages_sgl(nseg);
+	size_t npages = nvme_npages(size, dev);
+	size_t alloc_size = sizeof(__le64 *);
 
-	if (use_sgl)
-		alloc_size = sizeof(__le64 *) * nvme_pci_npages_sgl(nseg);
-	else
-		alloc_size = sizeof(__le64 *) * nvme_npages(size, dev);
+	alloc_size *= (npages_sgl > npages ? npages_sgl : npages);
 
 	return alloc_size + sizeof(struct scatterlist) * nseg;
 }
@@ -2819,7 +2818,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	 * command we support.
 	 */
 	alloc_size = nvme_pci_iod_alloc_size(dev, NVME_MAX_KB_SZ,
-						NVME_MAX_SEGS, true);
+						NVME_MAX_SEGS);
 	WARN_ON_ONCE(alloc_size > PAGE_SIZE);
 
 	dev->iod_mempool = mempool_create_node(1, mempool_kmalloc,
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/4] nvme-pci: use max of PRP or SGL for iod size

[Keith Busch]

On Tue, Jul 07, 2020 at 05:58:30PM -0700, Chaitanya Kulkarni wrote:
>  static size_t nvme_pci_iod_alloc_size(struct nvme_dev *dev,
> -		unsigned int size, unsigned int nseg, bool use_sgl)
> +		unsigned int size, unsigned int nseg)
>  {
> -	size_t alloc_size;
> +	size_t npages_sgl = nvme_pci_npages_sgl(nseg);
> +	size_t npages = nvme_npages(size, dev);
> +	size_t alloc_size = sizeof(__le64 *);
>  
> -	if (use_sgl)
> -		alloc_size = sizeof(__le64 *) * nvme_pci_npages_sgl(nseg);
> -	else
> -		alloc_size = sizeof(__le64 *) * nvme_npages(size, dev);
> +	alloc_size *= (npages_sgl > npages ? npages_sgl : npages);

This would look better as:

	alloc_size *= max(npages, npages_sgl);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/4] nvme-pci: use max of PRP or SGL for iod size

[Chaitanya Kulkarni]

On 7/8/20 15:27, Keith Busch wrote:
>> -	else
>> -		alloc_size = sizeof(__le64 *) * nvme_npages(size, dev);
>> +	alloc_size *= (npages_sgl > npages ? npages_sgl : npages);
> This would look better as:
> 
> 	alloc_size *= max(npages, npages_sgl);
> 

Okay, I'll fix it.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/4] nvme-core: fix checkpatch warnings

[Chaitanya Kulkarni]

This patch fixes couple of warnings (comment fix and
unsigned-> unsigned int) reported by the checkpatch.pl which
were present in the original code.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 2 +-
 drivers/nvme/host/nvme.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e0b47e77cbca..a35725aa6144 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2344,7 +2344,7 @@ EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned dev_page_min;
+	unsigned int dev_page_min;
 	int ret;
 
 	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4a56897e9081..9aa1d17f06e8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -39,7 +39,7 @@ extern unsigned int admin_timeout;
 
 /*
  * Default to a 4K page size, with the intention to update this
- * path in the future to accomodate architectures with differing
+ * path in the future to accommodate architectures with differing
  * kernel and IO page sizes.
  */
 #define NVME_CTRL_PAGE_SHIFT	12
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme