[PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Christoph Hellwig]

nvme_alloc_host_mem currently contains two loops that are interwinded,
and the outer retry loop turns out to be broken.  Fix this by untangling
the two.

Based on a report an initial patch from Akinobu Mita.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
Tested-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/nvme/host/pci.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 11874afb2422..e3252bcbc58e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1616,17 +1616,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;
@@ -1652,15 +1650,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;
@@ -1679,15 +1671,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;
@@ -1715,8 +1720,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))
-- 
2.11.0

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

[Christoph Hellwig]

From: Akinobu Mita <akinobu.mita@gmail.com>

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.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
[hch: rebased]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.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 e3252bcbc58e..6d2acf06c180 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1680,7 +1680,7 @@ 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);
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE * MAX_ORDER_NR_PAGES);
 	     chunk_size >= PAGE_SIZE * 2;
 	     chunk_size /= 2) {
 		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
-- 
2.11.0

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Christoph Hellwig]

We want to catch command execution errors when resetting the device, so
propagate errors from the Set Features when setting up the host memory
buffer.  We keep ignoring memory allocation failures, as the spec
clearly says that the controller must work without a host memory buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
---
 drivers/nvme/host/pci.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6d2acf06c180..06ea46801578 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1693,12 +1693,13 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	return -ENOMEM;
 }
 
-static void nvme_setup_host_mem(struct nvme_dev *dev)
+static int nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
 	u64 preferred = (u64)dev->ctrl.hmpre * 4096;
 	u64 min = (u64)dev->ctrl.hmmin * 4096;
 	u32 enable_bits = NVME_HOST_MEM_ENABLE;
+	int ret = 0;
 
 	preferred = min(preferred, max);
 	if (min > max) {
@@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			"min host memory (%lld MiB) above limit (%d MiB).\n",
 			min >> ilog2(SZ_1M), max_host_mem_size_mb);
 		nvme_free_host_mem(dev);
-		return;
+		return 0;
 	}
 
 	/*
@@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 		if (nvme_alloc_host_mem(dev, min, preferred)) {
 			dev_warn(dev->ctrl.device,
 				"failed to allocate host memory buffer.\n");
-			return;
+			return 0; /* controller must work without HMB */
 		}
 
 		dev_info(dev->ctrl.device,
@@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
-	if (nvme_set_host_mem(dev, enable_bits))
+	ret = nvme_set_host_mem(dev, enable_bits);
+	if (ret)
 		nvme_free_host_mem(dev);
+	return ret;
 }
 
 static int nvme_setup_io_queues(struct nvme_dev *dev)
@@ -2176,8 +2179,11 @@ static void nvme_reset_work(struct work_struct *work)
 				 "unable to allocate dma for dbbuf\n");
 	}
 
-	if (dev->ctrl.hmpre)
-		nvme_setup_host_mem(dev);
+	if (dev->ctrl.hmpre) {
+		result = nvme_setup_host_mem(dev);
+		if (result < 0)
+			goto out;
+	}
 
 	result = nvme_setup_io_queues(dev);
 	if (result)
-- 
2.11.0

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Holger Hoffstätte]

On 09/06/17 15:55, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.

Maybe I'm missing something but I think you forgot to update a bunch of
function signatures in terms of return type. Changing "return" to "return 0"
IMHO makes little sense otherwise.

-h

> @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			"min host memory (%lld MiB) above limit (%d MiB).\n",
>  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
>  		nvme_free_host_mem(dev);
> -		return;
> +		return 0;

[..]

> @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  		if (nvme_alloc_host_mem(dev, min, preferred)) {
>  			dev_warn(dev->ctrl.device,
>  				"failed to allocate host memory buffer.\n");
> -			return;
> +			return 0; /* controller must work without HMB */

[..]

> @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			dev->host_mem_size >> ilog2(SZ_1M));
>  	}
>  
> -	if (nvme_set_host_mem(dev, enable_bits))
> +	ret = nvme_set_host_mem(dev, enable_bits);
> +	if (ret)
>  		nvme_free_host_mem(dev);
> +	return ret;

Re: [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:29PM +0200, Christoph Hellwig wrote:
> nvme_alloc_host_mem currently contains two loops that are interwinded,
> and the outer retry loop turns out to be broken.  Fix this by untangling
> the two.
> 
> Based on a report an initial patch from Akinobu Mita.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
> Tested-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: stable@vger.kernel.org
> ---

Looks good.

Reviewed-by: Keith Busch <keith.busch@intel.com>

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

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:30PM +0200, Christoph Hellwig wrote:
> From: Akinobu Mita <akinobu.mita@gmail.com>
> 
> 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.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> [hch: rebased]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@vger.kernel.org
> ---

Looks good.

Reviewed-by: Keith Busch <keith.busch@intel.com>

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017 at 04:19:43PM +0200, Holger Hoffst�tte wrote:
> On 09/06/17 15:55, Christoph Hellwig wrote:
> > We want to catch command execution errors when resetting the device, so
> > propagate errors from the Set Features when setting up the host memory
> > buffer.  We keep ignoring memory allocation failures, as the spec
> > clearly says that the controller must work without a host memory buffer.
> 
> Maybe I'm missing something but I think you forgot to update a bunch of
> function signatures in terms of return type. Changing "return" to "return 0"
> IMHO makes little sense otherwise.
> 
> -h

The sig is updated:

 -static void nvme_setup_host_mem(struct nvme_dev *dev)
 +static int nvme_setup_host_mem(struct nvme_dev *dev)

The diff is just showing the old sig.

> > @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			"min host memory (%lld MiB) above limit (%d MiB).\n",
> >  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
> >  		nvme_free_host_mem(dev);
> > -		return;
> > +		return 0;
> 
> [..]
> 
> > @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  		if (nvme_alloc_host_mem(dev, min, preferred)) {
> >  			dev_warn(dev->ctrl.device,
> >  				"failed to allocate host memory buffer.\n");
> > -			return;
> > +			return 0; /* controller must work without HMB */
> 
> [..]
> 
> > @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			dev->host_mem_size >> ilog2(SZ_1M));
> >  	}
> >  
> > -	if (nvme_set_host_mem(dev, enable_bits))
> > +	ret = nvme_set_host_mem(dev, enable_bits);
> > +	if (ret)
> >  		nvme_free_host_mem(dev);
> > +	return ret;

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:31PM +0200, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@vger.kernel.org

Looks good.

Reviewed-by: Keith Busch <keith.busch@intel.com>