nvme-pci: add module parameter for io queue depth

PATCH New weiping zhang 6 participants 8 messages Jul 7, 2017 – Jul 10, 2017 NVMe

lore

Share & tools

lore

weiping zhang

author

2017-07-07 09:05 UTC #0 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

Adjust io queue depth more easily.

Signed-off-by weiping zhang <zhangweiping at didichuxing.com>

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

drivers/nvme/host/pci.c +5 -2

index 951042a..f1ac0f8 100644
@@ -48,7 +48,6 @@
 
 #include "nvme.h"
 
-#define NVME_Q_DEPTH		1024
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))

@@ -66,6 +65,10 @@ static bool use_cmb_sqes = true;
 module_param(use_cmb_sqes, bool, 0644);
 MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes");
 
+static int io_queue_depth = 1024;
+module_param(io_queue_depth, int, 0644);
+MODULE_PARM_DESC(io_queue_depth, "set io queue depth");
+
 static struct workqueue_struct *nvme_workq;
 
 struct nvme_dev;

@@ -1730,7 +1733,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
-	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, NVME_Q_DEPTH);
+	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, io_queue_depth);
 	dev->db_stride = 1 << NVME_CAP_STRIDE(cap);
 	dev->dbs = dev->bar + 4096;

Signature · 1 line

2.9.4

Christoph Hellwig

maintainer

2017-07-07 13:50 UTC #1 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

Quoted 3 lines · weiping zhang

Adjust io queue depth more easily.
Signed-off-by weiping zhang <zhangweiping at didichuxing.com>

Sounds useful in general, the only thin I'm worried about is that this will affect all drives in the system, which might have very different characteristics. But I can't really think of a better approach.

Jens Axboe

2017-07-07 14:56 UTC #2 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

Quoted 9 lines · Christoph Hellwig

Quoted 3 lines · weiping zhang
Adjust io queue depth more easily.
Signed-off-by weiping zhang <zhangweiping at didichuxing.com>
Sounds useful in general, the only thin I'm worried about is that this will affect all drives in the system, which might have very different characteristics. But I can't really think of a better approach.

That's a generic problem that all module parameters have, and it's sometimes quite annoying. But that should be fixed separately. I'm fine with this patch, it's an improvement over what we have.

Signature · 1 line

Jens Axboe

Jens Axboe

maintainer

2017-07-07 14:56 UTC #3 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

Quoted 3 lines · weiping zhang
Adjust io queue depth more easily.
Signed-off-by weiping zhang <zhangweiping at didichuxing.com>
Sounds useful in general, the only thin I'm worried about is that this will affect all drives in the system, which might have very different characteristics. But I can't really think of a better approach.

That's a generic problem that all module parameters have, and it's sometimes quite annoying. But that should be fixed separately. I'm fine with this patch, it's an improvement over what we have.

Signature · 1 line

Jens Axboe

Keith Busch

2017-07-07 15:29 UTC #4 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

In reply to weiping zhang

Quoted 31 lines · weiping zhang

Adjust io queue depth more easily.

Signed-off-by weiping zhang <zhangweiping at didichuxing.com>

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

drivers/nvme/host/pci.c +4 -1

index 951042a..f1ac0f8 100644
@@ -48,7 +48,6 @@
 
 #include "nvme.h"
 
-#define NVME_Q_DEPTH		1024
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))

@@ -66,6 +65,10 @@ static bool use_cmb_sqes = true;
 module_param(use_cmb_sqes, bool, 0644);
 MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes");
 
+static int io_queue_depth = 1024;
+module_param(io_queue_depth, int, 0644);
+MODULE_PARM_DESC(io_queue_depth, "set io queue depth");
+
 static struct workqueue_struct *nvme_workq;
 
 struct nvme_dev;

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1736

Quoted 5 lines

drivers/nvme/host/pci.c +1 -1

@@ -1730,7 +1733,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
-	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, NVME_Q_DEPTH);
+	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, io_queue_depth);

We need to be a little more careful about this field if users can write whatever they want. It's gotta be >= 2 or else trouble will occur.

weiping zhang

author

2017-07-07 16:53 UTC #5 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

Quoted 9 lines · Keith Busch

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1736
Quoted 5 lines · weiping zhang
Adjust io queue depth more easily.
drivers/nvme/host/pci.c +1 -1
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
-	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, NVME_Q_DEPTH);
+	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, io_queue_depth);
We need to be a little more careful about this field if users can write whatever they want. It's gotta be >= 2 or else trouble will occur.

ok, I will send patch v2, thanks all.

Sagi Grimberg

maintainer

2017-07-10 05:54 UTC #6 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

In reply to Keith Busch

Quoted 10 lines

drivers/nvme/host/pci.c +4 -0

+static int io_queue_depth = 1024;
+module_param(io_queue_depth, int, 0644);
+MODULE_PARM_DESC(io_queue_depth, "set io queue depth");
+
  static struct workqueue_struct *nvme_workq;
 
  struct nvme_dev;

@@ -1730,7 +1733,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)

Quoted 7 lines

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 1736
Quoted 4 lines
drivers/nvme/host/pci.c +1 -1
  	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);
 
-	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, NVME_Q_DEPTH);
+	dev->q_depth = min_t(int, NVME_CAP_MQES(cap) + 1, io_queue_depth);
We need to be a little more careful about this field if users can write whatever they want. It's gotta be >= 2 or else trouble will occur.

Agreed, please resend the patch. Perhaps a good way is to use module_param_cb() to verify the input value is >= 2?

weiping zhang

author

2017-07-10 08:02 UTC #7 lore raw

PATCH 1/1 nvme-pci: add module parameter for io queue depth

Quoted 9 lines · Sagi Grimberg

Quoted 6 lines
drivers/nvme/host/pci.c static bool use_cmb_sqes = true; · line 70
Quoted 4 lines
drivers/nvme/host/pci.c +3 -0
+static int io_queue_depth = 1024;
+module_param(io_queue_depth, int, 0644);
+MODULE_PARM_DESC(io_queue_depth, "set io queue depth");
We need to be a little more careful about this field if users can write whatever they want. It's gotta be >= 2 or else trouble will occur.
Agreed, please resend the patch. Perhaps a good way is to use module_param_cb() to verify the input value is >= 2?

thanks so much, I will send patch v3.