* [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback
2017-09-06 13:55 nvme host memory buffer fixes and updates Christoph Hellwig
@ 2017-09-06 13:55 ` Christoph Hellwig
2017-09-06 20:12 ` Keith Busch
2017-09-06 13:55 ` [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation Christoph Hellwig
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:55 UTC (permalink / raw)
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 at lst.de>
Reported-by: Akinobu Mita <akinobu.mita at gmail.com>
Tested-by: Akinobu Mita <akinobu.mita at gmail.com>
Cc: stable at 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback
2017-09-06 13:55 ` [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback Christoph Hellwig
@ 2017-09-06 20:12 ` Keith Busch
0 siblings, 0 replies; 11+ messages in thread
From: Keith Busch @ 2017-09-06 20:12 UTC (permalink / raw)
On Wed, Sep 06, 2017@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 at lst.de>
> Reported-by: Akinobu Mita <akinobu.mita at gmail.com>
> Tested-by: Akinobu Mita <akinobu.mita at gmail.com>
> Cc: stable at vger.kernel.org
> ---
Looks good.
Reviewed-by: Keith Busch <keith.busch at intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation
2017-09-06 13:55 nvme host memory buffer fixes and updates Christoph Hellwig
2017-09-06 13:55 ` [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback Christoph Hellwig
@ 2017-09-06 13:55 ` Christoph Hellwig
2017-09-06 20:13 ` Keith Busch
2017-09-06 13:55 ` [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup Christoph Hellwig
2017-09-06 13:55 ` [PATCH 4/4] nvme-pci: implement the HMB entry number and size limitations Christoph Hellwig
3 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:55 UTC (permalink / raw)
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 at gmail.com>
[hch: rebased]
Signed-off-by: Christoph Hellwig <hch at lst.de>
Cc: stable at 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation
2017-09-06 13:55 ` [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation Christoph Hellwig
@ 2017-09-06 20:13 ` Keith Busch
0 siblings, 0 replies; 11+ messages in thread
From: Keith Busch @ 2017-09-06 20:13 UTC (permalink / raw)
On Wed, Sep 06, 2017@03:55:30PM +0200, Christoph Hellwig wrote:
> From: Akinobu Mita <akinobu.mita at 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 at gmail.com>
> [hch: rebased]
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Cc: stable at vger.kernel.org
> ---
Looks good.
Reviewed-by: Keith Busch <keith.busch at intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup
2017-09-06 13:55 nvme host memory buffer fixes and updates Christoph Hellwig
2017-09-06 13:55 ` [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback Christoph Hellwig
2017-09-06 13:55 ` [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation Christoph Hellwig
@ 2017-09-06 13:55 ` Christoph Hellwig
2017-09-06 14:19 ` Holger Hoffstätte
2017-09-06 21:49 ` Keith Busch
2017-09-06 13:55 ` [PATCH 4/4] nvme-pci: implement the HMB entry number and size limitations Christoph Hellwig
3 siblings, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:55 UTC (permalink / raw)
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 at lst.de>
Cc: stable at 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup
2017-09-06 13:55 ` [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup Christoph Hellwig
@ 2017-09-06 14:19 ` Holger Hoffstätte
2017-09-06 20:27 ` Keith Busch
2017-09-06 21:49 ` Keith Busch
1 sibling, 1 reply; 11+ messages in thread
From: Holger Hoffstätte @ 2017-09-06 14:19 UTC (permalink / raw)
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;
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup
2017-09-06 14:19 ` Holger Hoffstätte
@ 2017-09-06 20:27 ` Keith Busch
0 siblings, 0 replies; 11+ messages in thread
From: Keith Busch @ 2017-09-06 20:27 UTC (permalink / raw)
On Wed, Sep 06, 2017@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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup
2017-09-06 13:55 ` [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup Christoph Hellwig
2017-09-06 14:19 ` Holger Hoffstätte
@ 2017-09-06 21:49 ` Keith Busch
1 sibling, 0 replies; 11+ messages in thread
From: Keith Busch @ 2017-09-06 21:49 UTC (permalink / raw)
On Wed, Sep 06, 2017@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 at lst.de>
> Cc: stable at vger.kernel.org
Looks good.
Reviewed-by: Keith Busch <keith.busch at intel.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] nvme-pci: implement the HMB entry number and size limitations
2017-09-06 13:55 nvme host memory buffer fixes and updates Christoph Hellwig
` (2 preceding siblings ...)
2017-09-06 13:55 ` [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup Christoph Hellwig
@ 2017-09-06 13:55 ` Christoph Hellwig
2017-09-06 20:11 ` Keith Busch
3 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2017-09-06 13:55 UTC (permalink / raw)
Adds support for the new Host Memory Buffer Minimum Descriptor Entry Size
and Host Memory Maximum Descriptors Entries field that were added in
TP 4002 HMB Enhancements. These allow the controller to advertise
limits for the usual number of segments in the host memory buffer, as
well as a minimum usable per-segment size.
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
drivers/nvme/host/core.c | 2 ++
drivers/nvme/host/nvme.h | 3 +++
drivers/nvme/host/pci.c | 6 +++++-
include/linux/nvme.h | 4 +++-
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 277a7a02cba5..407fc1c80d5b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1897,6 +1897,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
ctrl->cntlid = le16_to_cpu(id->cntlid);
ctrl->hmpre = le32_to_cpu(id->hmpre);
ctrl->hmmin = le32_to_cpu(id->hmmin);
+ ctrl->hmminds = le32_to_cpu(id->hmminds);
+ ctrl->hmmaxd = le16_to_cpu(id->hmmaxd);
}
kfree(id);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a19a587d60ed..432c543beaed 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -176,8 +176,11 @@ struct nvme_ctrl {
u64 ps_max_latency_us;
bool apst_enabled;
+ /* PCIe only: */
u32 hmpre;
u32 hmmin;
+ u32 hmminds;
+ u16 hmmaxd;
/* Fabrics only */
u16 sqsize;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 06ea46801578..ef9093f617ac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1628,6 +1628,10 @@ static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
tmp = (preferred + chunk_size - 1);
do_div(tmp, chunk_size);
max_entries = tmp;
+
+ if (dev->ctrl.hmmaxd && dev->ctrl.hmmaxd < max_entries)
+ max_entries = dev->ctrl.hmmaxd;
+
descs = kcalloc(max_entries, sizeof(*descs), GFP_KERNEL);
if (!descs)
goto out;
@@ -1681,7 +1685,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
/* start big and work our way down */
for (chunk_size = min_t(u64, preferred, PAGE_SIZE * MAX_ORDER_NR_PAGES);
- chunk_size >= PAGE_SIZE * 2;
+ chunk_size >= max_t(u32, dev->ctrl.hmminds * 4096, PAGE_SIZE * 2);
chunk_size /= 2) {
if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
if (!min || dev->host_mem_size >= min)
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 05560c2ecc83..c46f42269021 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -226,7 +226,9 @@ struct nvme_id_ctrl {
__le16 mntmt;
__le16 mxtmt;
__le32 sanicap;
- __u8 rsvd332[180];
+ __le32 hmminds;
+ __le16 hmmaxd;
+ __u8 rsvd338[174];
__u8 sqes;
__u8 cqes;
__le16 maxcmd;
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread