[PATCH v2 0/2] Some improvements for NVMe

[PATCH v2 0/2] Some improvements for NVMe

[Baolin Wang]

Hi,

The fisrt patch adds Arbitration Burst support, and another one is
a small improvement. Please help to review. Thanks.

Changes from v1:
 - Add new parameter to decide if use the controller's recommended
 arbitration burst.
 - Add arbitration burst mask in case overflow.
 - Do not export the nvme_set_arbitration_burst().
 - Add reviewed tag from Sagi.
 - Drop patch 2 in v1 patch set.

Baolin Wang (2):
  nvme: Add Arbitration Burst support
  nvme: Use USEC_PER_SEC instead of magic numbers

 drivers/nvme/host/core.c | 31 ++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 include/linux/nvme.h     |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH v2 1/2] nvme: Add Arbitration Burst support

[Baolin Wang]

From the NVMe spec, "In order to make efficient use of the non-volatile
memory, it is often advantageous to execute multiple commands from a
Submission Queue in parallel. For Submission Queues that are using
weighted round robin with urgent priority class or round robin
arbitration, host software may configure an Arbitration Burst setting".
Thus add Arbitration Burst setting support.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 drivers/nvme/host/core.c | 29 +++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 include/linux/nvme.h     |  1 +
 3 files changed, 31 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2c5bc4..f5f882f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -61,6 +61,10 @@
 module_param(streams, bool, 0644);
 MODULE_PARM_DESC(streams, "turn on support for Streams write directives");
 
+static bool use_rab;
+module_param(use_rab, bool, 0644);
+MODULE_PARM_DESC(use_rab, "use controller's recommended arbitration burst");
+
 /*
  * nvme_wq - hosts nvme related works that are not reset or delete
  * nvme_reset_wq - hosts nvme reset works
@@ -1241,6 +1245,28 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 }
 EXPORT_SYMBOL_GPL(nvme_set_queue_count);
 
+static void nvme_set_arbitration_burst(struct nvme_ctrl *ctrl)
+{
+	u32 result;
+	int status;
+
+	if (!use_rab || !ctrl->rab)
+		return;
+
+	/*
+	 * The Arbitration Burst setting indicates the maximum number of
+	 * commands that the controller may launch at one time from a
+	 * particular Submission Queue. It is recommended that host software
+	 * configure the Arbitration Burst setting as close to the recommended
+	 * value by the controller as possible.
+	 */
+	status = nvme_set_features(ctrl, NVME_FEAT_ARBITRATION,
+				   ctrl->rab & NVME_FEAT_ARBITRATION_MASK,
+				   NULL, 0, &result);
+	if (status)
+		dev_warn(ctrl->device, "Failed to set Arbitration Burst\n");
+}
+
 #define NVME_AEN_SUPPORTED \
 	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | \
 	 NVME_AEN_CFG_ANA_CHANGE | NVME_AEN_CFG_DISC_CHANGE)
@@ -2953,6 +2979,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	} else
 		ctrl->shutdown_timeout = shutdown_timeout;
 
+	ctrl->rab = id->rab;
+	nvme_set_arbitration_burst(ctrl);
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c0f4226..bc424c5 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -246,6 +246,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	u8 rab;
 	u16 wctemp;
 	u16 cctemp;
 	u32 oaes;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 5ce51ab..fa629c8 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -909,6 +909,7 @@ enum {
 	NVME_SQ_PRIO_MEDIUM	= (2 << 1),
 	NVME_SQ_PRIO_LOW	= (3 << 1),
 	NVME_FEAT_ARBITRATION	= 0x01,
+	NVME_FEAT_ARBITRATION_MASK = 0x7,
 	NVME_FEAT_POWER_MGMT	= 0x02,
 	NVME_FEAT_LBA_RANGE	= 0x03,
 	NVME_FEAT_TEMP_THRESH	= 0x04,
-- 
1.8.3.1

Re: [PATCH v2 1/2] nvme: Add Arbitration Burst support

[Christoph Hellwig]

On Wed, Jun 24, 2020 at 02:49:57PM +0800, Baolin Wang wrote:
> >From the NVMe spec, "In order to make efficient use of the non-volatile
> memory, it is often advantageous to execute multiple commands from a
> Submission Queue in parallel. For Submission Queues that are using
> weighted round robin with urgent priority class or round robin
> arbitration, host software may configure an Arbitration Burst setting".
> Thus add Arbitration Burst setting support.

What is the value add of doing this in the kernel?  Wouldn't a nvme-cli
subcommand to just set the arbitration burst to the recommended value,
either as a saved or current value be both more useful and also cause
less kernel bloat?

Re: [PATCH v2 1/2] nvme: Add Arbitration Burst support

[Baolin Wang]

On Wed, Jun 24, 2020 at 08:52:58AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 24, 2020 at 02:49:57PM +0800, Baolin Wang wrote:
> > >From the NVMe spec, "In order to make efficient use of the non-volatile
> > memory, it is often advantageous to execute multiple commands from a
> > Submission Queue in parallel. For Submission Queues that are using
> > weighted round robin with urgent priority class or round robin
> > arbitration, host software may configure an Arbitration Burst setting".
> > Thus add Arbitration Burst setting support.
> 
> What is the value add of doing this in the kernel?  Wouldn't a nvme-cli
> subcommand to just set the arbitration burst to the recommended value,
> either as a saved or current value be both more useful and also cause
> less kernel bloat?

Yes, that's another way to configure the RAB, but for some simple and
frequently-used usage cases, we just want to use the recommanded arbitration
burst as default without bothering the nvme-cli tool every reset time.

Re: [PATCH v2 1/2] nvme: Add Arbitration Burst support

[Christoph Hellwig]

On Wed, Jun 24, 2020 at 03:58:54PM +0800, Baolin Wang wrote:
> On Wed, Jun 24, 2020 at 08:52:58AM +0200, Christoph Hellwig wrote:
> > On Wed, Jun 24, 2020 at 02:49:57PM +0800, Baolin Wang wrote:
> > > >From the NVMe spec, "In order to make efficient use of the non-volatile
> > > memory, it is often advantageous to execute multiple commands from a
> > > Submission Queue in parallel. For Submission Queues that are using
> > > weighted round robin with urgent priority class or round robin
> > > arbitration, host software may configure an Arbitration Burst setting".
> > > Thus add Arbitration Burst setting support.
> > 
> > What is the value add of doing this in the kernel?  Wouldn't a nvme-cli
> > subcommand to just set the arbitration burst to the recommended value,
> > either as a saved or current value be both more useful and also cause
> > less kernel bloat?
> 
> Yes, that's another way to configure the RAB, but for some simple and
> frequently-used usage cases, we just want to use the recommanded arbitration
> burst as default without bothering the nvme-cli tool every reset time.

Please look up how saved features work in nvme.

[PATCH v2 2/2] nvme: Use USEC_PER_SEC instead of magic numbers

[Baolin Wang]

Use USEC_PER_SEC instead of magic numbers to make code more readable.

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f5f882f..405bafa 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2967,7 +2967,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
 	if (id->rtd3e) {
 		/* us -> s */
-		u32 transition_time = le32_to_cpu(id->rtd3e) / 1000000;
+		u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
 
 		ctrl->shutdown_timeout = clamp_t(unsigned int, transition_time,
 						 shutdown_timeout, 60);
-- 
1.8.3.1

Re: [PATCH v2 2/2] nvme: Use USEC_PER_SEC instead of magic numbers

[Christoph Hellwig]

Thanks,

applied to nvme-5.9.