... and cap the length to 256 to avoid oom-ing.
Signed-off-by: Filippo Sironi sironi@amazon.de --- src/hw/nvme.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/hw/nvme.c b/src/hw/nvme.c index 946487f4fd43..19a4e7a48d3c 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -20,6 +20,8 @@ #include "nvme.h" #include "nvme-int.h"
+#define min(a, b) ({ typeof(a) _a = a; typeof(b) _b = b; _a < _b ? _a : _b; }) + static void * zalloc_page_aligned(struct zone_s *zone, u32 size) { @@ -318,8 +320,10 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct nvme_cq *cq, u16 q_idx) { int rc; struct nvme_sqe *cmd_create_cq; + u16 length;
- rc = nvme_init_cq(ctrl, cq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + length = min(1 + (ctrl->reg->cap & 0xffff), NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + rc = nvme_init_cq(ctrl, cq, q_idx, length); if (rc) { goto err; } @@ -359,8 +363,10 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct nvme_sq *sq, u16 q_idx, struct { int rc; struct nvme_sqe *cmd_create_sq; + u16 length;
- rc = nvme_init_sq(ctrl, sq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe), cq); + length = min(1 + (ctrl->reg->cap & 0xffff), NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + rc = nvme_init_sq(ctrl, sq, q_idx, length, cq); if (rc) { goto err; }
On Tue, Oct 10, 2017 at 10:42:03PM +0200, Filippo Sironi wrote:
... and cap the length to 256 to avoid oom-ing.
Please provide more information on what this patch does. What additional capabilities does it provide? What could break if this patch is not applied?
-Kevin
On 12. Oct 2017, at 00:54, Kevin O'Connor kevin@koconnor.net wrote:
On Tue, Oct 10, 2017 at 10:42:03PM +0200, Filippo Sironi wrote:
... and cap the length to 256 to avoid oom-ing.
Please provide more information on what this patch does. What additional capabilities does it provide? What could break if this patch is not applied?
-Kevin
Kevin,
as the commit title says, the patch is mostly about using the advertized maximum number of entries in the I/O queues rather than using a fixed number (256). In addition, the patch caps the number of entries in the I/O queues to 256 since from my testing, SeaBIOS was oom-ing when attempting to allocate more.
I'm going to repost with a more verbose commit message.
Filippo Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Signed-off-by: Filippo Sironi sironi@amazon.de --- src/hw/nvme.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/hw/nvme.c b/src/hw/nvme.c index 946487f4fd43..19a4e7a48d3c 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -20,6 +20,8 @@ #include "nvme.h" #include "nvme-int.h"
+#define min(a, b) ({ typeof(a) _a = a; typeof(b) _b = b; _a < _b ? _a : _b; }) + static void * zalloc_page_aligned(struct zone_s *zone, u32 size) { @@ -318,8 +320,10 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct nvme_cq *cq, u16 q_idx) { int rc; struct nvme_sqe *cmd_create_cq; + u16 length;
- rc = nvme_init_cq(ctrl, cq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + length = min(1 + (ctrl->reg->cap & 0xffff), NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + rc = nvme_init_cq(ctrl, cq, q_idx, length); if (rc) { goto err; } @@ -359,8 +363,10 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct nvme_sq *sq, u16 q_idx, struct { int rc; struct nvme_sqe *cmd_create_sq; + u16 length;
- rc = nvme_init_sq(ctrl, sq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe), cq); + length = min(1 + (ctrl->reg->cap & 0xffff), NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + rc = nvme_init_sq(ctrl, sq, q_idx, length, cq); if (rc) { goto err; }
On Thu, Oct 12, 2017 at 12:42:34AM +0200, Filippo Sironi wrote:
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Okay, thanks. So, it sounds like it is a bug fix (as it could prevent a failure if the hardware queue depth is less than 256).
I'd prefer not to introduce min()/max() macros to individual driver files. (I'm fine with a patch that globally introduces min/max, but I don't think we should be adding them piecemeal.) Are you okay with the patch below instead?
-Kevin
Author: Filippo Sironi sironi@amazon.de Date: Thu Oct 12 00:42:34 2017 +0200
nvme: Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Signed-off-by: Filippo Sironi sironi@amazon.de
diff --git a/src/hw/nvme.c b/src/hw/nvme.c index 946487f..e6d739d 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -318,8 +318,11 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct nvme_cq *cq, u16 q_idx) { int rc; struct nvme_sqe *cmd_create_cq; + u16 length = 1 + (ctrl->reg->cap & 0xffff); + if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe)) + length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
- rc = nvme_init_cq(ctrl, cq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe)); + rc = nvme_init_cq(ctrl, cq, q_idx, length); if (rc) { goto err; } @@ -359,8 +362,11 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct nvme_sq *sq, u16 q_idx, struct { int rc; struct nvme_sqe *cmd_create_sq; + u16 length = 1 + (ctrl->reg->cap & 0xffff); + if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe)) + length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
- rc = nvme_init_sq(ctrl, sq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe), cq); + rc = nvme_init_sq(ctrl, sq, q_idx, length, cq); if (rc) { goto err; }
On 14. Oct 2017, at 17:15, Kevin O'Connor kevin@koconnor.net wrote:
On Thu, Oct 12, 2017 at 12:42:34AM +0200, Filippo Sironi wrote:
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Okay, thanks. So, it sounds like it is a bug fix (as it could prevent a failure if the hardware queue depth is less than 256).
I'd prefer not to introduce min()/max() macros to individual driver files. (I'm fine with a patch that globally introduces min/max, but I don't think we should be adding them piecemeal.) Are you okay with the patch below instead?
-Kevin
Looks good to me.
Thanks, Filippo
Author: Filippo Sironi sironi@amazon.de Date: Thu Oct 12 00:42:34 2017 +0200
nvme: Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Signed-off-by: Filippo Sironi sironi@amazon.de
diff --git a/src/hw/nvme.c b/src/hw/nvme.c index 946487f..e6d739d 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -318,8 +318,11 @@ nvme_create_io_cq(struct nvme_ctrl *ctrl, struct nvme_cq *cq, u16 q_idx) { int rc; struct nvme_sqe *cmd_create_cq;
- u16 length = 1 + (ctrl->reg->cap & 0xffff);
- if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
- rc = nvme_init_cq(ctrl, cq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe));
- rc = nvme_init_cq(ctrl, cq, q_idx, length); if (rc) { goto err; }
@@ -359,8 +362,11 @@ nvme_create_io_sq(struct nvme_ctrl *ctrl, struct nvme_sq *sq, u16 q_idx, struct { int rc; struct nvme_sqe *cmd_create_sq;
- u16 length = 1 + (ctrl->reg->cap & 0xffff);
- if (length > NVME_PAGE_SIZE / sizeof(struct nvme_cqe))
length = NVME_PAGE_SIZE / sizeof(struct nvme_cqe);
- rc = nvme_init_sq(ctrl, sq, q_idx, NVME_PAGE_SIZE / sizeof(struct nvme_cqe), cq);
- rc = nvme_init_sq(ctrl, sq, q_idx, length, cq); if (rc) { goto err; }
Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Sun, Oct 15, 2017 at 10:30:29AM +0000, Sironi, Filippo wrote:
On 14. Oct 2017, at 17:15, Kevin O'Connor kevin@koconnor.net wrote:
On Thu, Oct 12, 2017 at 12:42:34AM +0200, Filippo Sironi wrote:
Use the Maximum Queue Entries Supported (MQES) to initialize I/O queues depth rather than picking a fixed number (256) which might not be supported by some NVMe controllers (the NVMe specification says that an NVMe controller may support any number between 2 to 4096).
Still cap the I/O queues depth to 256 since, during my testing, SeaBIOS was running out of memory when using something higher than 256 (4096 on the NVMe controller that I've had a chance to try).
Okay, thanks. So, it sounds like it is a bug fix (as it could prevent a failure if the hardware queue depth is less than 256).
I'd prefer not to introduce min()/max() macros to individual driver files. (I'm fine with a patch that globally introduces min/max, but I don't think we should be adding them piecemeal.) Are you okay with the patch below instead?
-Kevin
Looks good to me.
Thanks. I committed this change.
-Kevin
-
Filippo Sironi -
Kevin O'Connor -
Sironi, Filippo