nvme host memory buffer fixes and updates
Subsystems (2)
Christoph Hellwig
maintainerHi all,
this series fixes a few bugs in the HMB code, and adds support for two trivial new tunables exposed by some devices.
Christoph Hellwig
maintainerPATCH 1/4 nvme-pci: fix host memory buffer allocation fallback
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.
--- drivers/nvme/host/pci.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)
drivers/nvme/host/pci.c +30 -18
@@ -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))
Signature · 1 line
2.11.0
PATCH 1/4 nvme-pci: fix host memory buffer allocation fallback
Quoted 11 lines · Christoph Hellwig
nvme_alloc_host_memcurrently 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 <[email protected]>Reported-by Akinobu Mita <[email protected]>Tested-by Akinobu Mita <[email protected]>---
Looks good.
Christoph Hellwig
maintainerPATCH 2/4 nvme-pci: use appropriate initial chunk size for HMB allocation
In reply to Christoph HellwigFrom: Akinobu Mita <[email protected]>
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.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -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)) {
Signature · 1 line
2.11.0
PATCH 2/4 nvme-pci: use appropriate initial chunk size for HMB allocation
Quoted 8 lines · Christoph Hellwig
From: Akinobu Mita <[email protected]>
The initial chunk size for host memory buffer allocation is currently
PAGE_SIZE<<MAX_ORDER.MAX_ORDERorder allocation is usually failed withoutCONFIG_DMA_CMA. So the HMB allocation is retried with chunk sizePAGE_SIZE<< (MAX_ORDER- 1) in general, but there is no problem if the retry allocation works correctly.
Quoted 5 lines
Signed-off-by Akinobu Mita <[email protected]>[hch: rebased]Signed-off-by Christoph Hellwig <[email protected]>---
Looks good.
Christoph Hellwig
maintainerPATCH 3/4 nvme-pci: propagate (some) errors from host memory buffer setup
In reply to Christoph HellwigWe 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.
--- drivers/nvme/host/pci.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/nvme/host/pci.c +12 -6
@@ -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)
Signature · 1 line
2.11.0
PATCH 3/4 nvme-pci: propagate (some) errors from host memory buffer setup
Quoted 4 lines · 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.
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
Quoted 6 lines
drivers/nvme/host/pci.c +1
-1@@ -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 ;
[..]
Quoted 6 lines
drivers/nvme/host/pci.c +1
-1@@ -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 */
[..]
Quoted 9 lines
drivers/nvme/host/pci.c +3
-1@@ -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;
PATCH 3/4 nvme-pci: propagate (some) errors from host memory buffer setup
Quoted 11 lines · Holger Hoffst�tte
Quoted 4 lines · 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.
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.
Quoted 27 lines
Quoted 6 lines
drivers/nvme/host/pci.c +1
-1@@ -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 ;[..]
Quoted 6 lines
drivers/nvme/host/pci.c +1
-1@@ -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 */ [..]
Quoted 9 lines
drivers/nvme/host/pci.c +3
-1@@ -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;
PATCH 3/4 nvme-pci: propagate (some) errors from host memory buffer setup
In reply to Christoph HellwigQuoted 7 lines · 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 <[email protected]>
Looks good.
Christoph Hellwig
maintainerPATCH 4/4 nvme-pci: implement the HMB entry number and size limitations
In reply to Christoph HellwigAdds 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.
--- 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(-)
drivers/nvme/host/core.c +2 -0
@@ -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);
drivers/nvme/host/nvme.h +3 -0
@@ -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;
drivers/nvme/host/pci.c +5 -1
@@ -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)
include/linux/nvme.h +3 -1
@@ -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;
Signature · 1 line
2.11.0
PATCH 4/4 nvme-pci: implement the HMB entry number and size limitations
Quoted 7 lines · Christoph Hellwig
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>
Looks good.