[PATCH 0/2] nvme-pci: fix rety handling of host memory buffer allocation

[PATCH 0/2] nvme-pci: fix rety handling of host memory buffer allocation

[Akinobu Mita]

I found a problem in rety handling of host memory buffer allocation when
using v4.13 kernel with Toshiba NVMe device which supports HMB.

Akinobu Mita (2):
  nvme-pci: avoid arrary index out of bounds in retry HMB allocation
  nvme-pci: use appropriate initial chunk size for HMB allocation

 drivers/nvme/host/pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
-- 
2.7.4

[PATCH 1/2] nvme-pci: avoid arrary index out of bounds in retry HMB allocation

[Akinobu Mita]

The array index which is used for addressing each chunk of host memory
buffer is decreased to -1 in the middle of retrying HMB allocation with
a smaller chunk size.  This results memory corruptions and the head of
host memory buffer descriptor entry is lost.

This fixes it by resetting the index before restarting HMB allocation.
And this also removes unnecessary conditional statements for retrying
decision.

Fixes: 87ad72a59a38 ("nvme-pci: implement host memory buffer support")
Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 drivers/nvme/host/pci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ea892e7..7e71cc9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1614,13 +1614,15 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	struct nvme_host_mem_buf_desc *descs;
 	u32 chunk_size, max_entries, len;
 	dma_addr_t descs_dma;
-	int i = 0;
+	int i;
 	void **bufs;
-	u64 size = 0, tmp;
+	u64 size, tmp;
 
 	/* start big and work our way down */
 	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
 retry:
+	i = 0;
+	size = 0;
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;
@@ -1633,7 +1635,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	if (!bufs)
 		goto out_free_descs;
 
-	for (size = 0; size < preferred; size += len) {
+	for (; size < preferred; size += len) {
 		dma_addr_t dma_addr;
 
 		len = min_t(u64, chunk_size, preferred - size);
@@ -1677,7 +1679,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 			descs_dma);
 out:
 	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
+	if (chunk_size >= PAGE_SIZE * 2) {
 		chunk_size /= 2;
 		goto retry;
 	}
-- 
2.7.4

[PATCH 1/2] nvme-pci: avoid arrary index out of bounds in retry HMB allocation

[Christoph Hellwig]

Hmm.  I think I did a clear beginners mistake in that code in
mixing up two loops.  Can you try the version below that just
adds a new function to separate the loops (and also gets rid of
the weird early fail heuristic):

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 74a124a06264..324b995a46ed 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1614,17 +1614,15 @@ static void nvme_free_host_mem(struct nvme_dev *dev)
 	dev->host_mem_descs = NULL;
 }
 
-static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
+		u32 chunk_size)
 {
 	struct nvme_host_mem_buf_desc *descs;
-	u32 chunk_size, max_entries, len;
+	u32 max_entries, len;
 	int i = 0;
 	void **bufs;
 	u64 size = 0, tmp;
 
-	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
-retry:
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;
@@ -1650,15 +1648,9 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 		i++;
 	}
 
-	if (!size || (min && size < min)) {
-		dev_warn(dev->ctrl.device,
-			"failed to allocate host memory buffer.\n");
+	if (!size)
 		goto out_free_bufs;
-	}
 
-	dev_info(dev->ctrl.device,
-		"allocated %lld MiB host memory buffer.\n",
-		size >> ilog2(SZ_1M));
 	dev->nr_host_mem_descs = i;
 	dev->host_mem_size = size;
 	dev->host_mem_descs = descs;
@@ -1677,15 +1669,28 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 out_free_descs:
 	kfree(descs);
 out:
-	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
-		chunk_size /= 2;
-		goto retry;
-	}
 	dev->host_mem_descs = NULL;
 	return -ENOMEM;
 }
 
+static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+{
+	u32 chunk_size;
+
+	/* start big and work our way down */
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	     chunk_size >= PAGE_SIZE * 2;
+	     chunk_size /= 2) {
+		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
+			if (!min || dev->host_mem_size >= min)
+				return 0;
+			nvme_free_host_mem(dev);
+		}
+	}
+
+	return -ENOMEM;
+}
+
 static void nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
@@ -1713,8 +1718,15 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 	}
 
 	if (!dev->host_mem_descs) {
-		if (nvme_alloc_host_mem(dev, min, preferred))
+		if (nvme_alloc_host_mem(dev, min, preferred)) {
+			dev_warn(dev->ctrl.device,
+				"failed to allocate host memory buffer.\n");
 			return;
+		}
+
+		dev_info(dev->ctrl.device,
+			"allocated %lld MiB host memory buffer.\n",
+			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
 	if (nvme_set_host_mem(dev, enable_bits))

[PATCH 1/2] nvme-pci: avoid arrary index out of bounds in retry HMB allocation

[Akinobu Mita]

2017-09-05 5:03 GMT+09:00 Christoph Hellwig <hch at lst.de>:
> Hmm.  I think I did a clear beginners mistake in that code in
> mixing up two loops.  Can you try the version below that just
> adds a new function to separate the loops (and also gets rid of
> the weird early fail heuristic):

I tested the patch and it works fine. (this patch isn't applied cleanly
to v4.13, but the conflict was resolved easily)

[PATCH 2/2] nvme-pci: use appropriate initial chunk size for HMB allocation

[Akinobu Mita]

The initial chunk size for host memory buffer allocation is currently
PAGE_SIZE << MAX_ORDER.  MAX_ORDER order allocation is usually failed
without CONFIG_DMA_CMA.  So the HMB allocation is retried with
chunk size PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no
problem if the retry allocation works correctly.

This change just avoids the retry and "failed to allocate host memory
buffer." warning in the most common case.

Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
---
 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 7e71cc9..49e2e7f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1619,7 +1619,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	u64 size, tmp;
 
 	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
+	chunk_size = min(preferred, (u64)PAGE_SIZE * MAX_ORDER_NR_PAGES);
 retry:
 	i = 0;
 	size = 0;
-- 
2.7.4

[PATCH 2/2] nvme-pci: use appropriate initial chunk size for HMB allocation

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

(It might need some context changes depending on the fate of the
previous patch, but I'll resend it together with other HMB bits if
needed)

[PATCH 2/2] nvme-pci: use appropriate initial chunk size for HMB allocation

[Akinobu Mita]

2017-09-05 5:05 GMT+09:00 Christoph Hellwig <hch at lst.de>:
> Looks fine,
>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
>
> (It might need some context changes depending on the fate of the
> previous patch, but I'll resend it together with other HMB bits if
> needed)

Sounds good.  Please send it together.