[PATCH 0/3] Some improvements for NVMe

[PATCH 0/3] Some improvements for NVMe

[Baolin Wang]

Hi,

The fisrt patch adds Arbitration Burst support, and other 2 patches
are small improvements. Please help to review. Thanks.

Baolin Wang (3):
  nvme: Add Arbitration Burst support
  nvme-pci: Add controller memory buffer supported macro
  nvme: Use USEC_PER_SEC instead of magic numbers

 drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  2 ++
 drivers/nvme/host/pci.c  |  4 +++-
 include/linux/nvme.h     |  1 +
 4 files changed, 30 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] 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 | 23 +++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 drivers/nvme/host/pci.c  |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2c5bc4..c3a60d8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1241,6 +1241,28 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 }
 EXPORT_SYMBOL_GPL(nvme_set_queue_count);
 
+void nvme_set_arbitration_burst(struct nvme_ctrl *ctrl)
+{
+	u32 result;
+	int status;
+
+	if (!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,
+				   NULL, 0, &result);
+	if (status)
+		dev_warn(ctrl->device, "Failed to set Arbitration Burst\n");
+}
+EXPORT_SYMBOL_GPL(nvme_set_arbitration_burst);
+
 #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 +2975,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	} else
 		ctrl->shutdown_timeout = shutdown_timeout;
 
+	ctrl->rab = id->rab;
 	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..3934426 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;
@@ -550,6 +551,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned int fid,
 		      unsigned int dword11, void *buffer, size_t buflen,
 		      u32 *result);
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
+void nvme_set_arbitration_burst(struct nvme_ctrl *ctrl);
 void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
 int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e2bacd3..4b4b7b7 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2111,6 +2111,8 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	
 	clear_bit(NVMEQ_ENABLED, &adminq->flags);
 
+	nvme_set_arbitration_burst(&dev->ctrl);
+
 	if (dev->cmb_use_sqes) {
 		result = nvme_cmb_qdepth(dev, nr_io_queues,
 				sizeof(struct nvme_command));
-- 
1.8.3.1

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

[Keith Busch]

On Tue, Jun 23, 2020 at 09:24:32PM +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.

But if the user changed it to something else that better matches how
they want to use queues, the driver is just going to undo that setting
on the next reset.

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

[Sagi Grimberg]

>>  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.
> 
> But if the user changed it to something else that better matches how
> they want to use queues, the driver is just going to undo that setting
> on the next reset.

Where do we do priority class arbitration? no one sets it

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

[Keith Busch]

On Tue, Jun 23, 2020 at 10:39:01AM -0700, Sagi Grimberg 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.
> > 
> > But if the user changed it to something else that better matches how
> > they want to use queues, the driver is just going to undo that setting
> > on the next reset.
> 
> Where do we do priority class arbitration? no one sets it

Not the priority class, we don't use WRR in this driver. The RR
arbitration can be set from user space and saved across controller
resets, like:

  # nvme set-feature -f 1 -v 3 --save

This patch would undo the saved feature value.

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

[Baolin Wang]

On Tue, Jun 23, 2020 at 11:01:08AM -0700, Keith Busch wrote:
> On Tue, Jun 23, 2020 at 10:39:01AM -0700, Sagi Grimberg 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.
> > > 
> > > But if the user changed it to something else that better matches how
> > > they want to use queues, the driver is just going to undo that setting
> > > on the next reset.
> > 
> > Where do we do priority class arbitration? no one sets it
> 
> Not the priority class, we don't use WRR in this driver. The RR
> arbitration can be set from user space and saved across controller
> resets, like:
> 
>   # nvme set-feature -f 1 -v 3 --save
> 
> This patch would undo the saved feature value.

OK, I understaood your concern. Now we will select the RR arbitration as default
in nvme_enable_ctrl(), but for some cases, we will not set the arbitration burst
values from userspace, and we still want to use the defaut arbitration burst that
recommended by the controller, taking into consideration any latency requirements.

So I'd like to add a parameter to decide if we can use the default arbitration
burst like below, how do yo think? Thanks.

static bool use_default_arb;
module_param(use_default_arb, bool, 0444);
MODULE_PARM_DESC(use_default_arb, "use default arbitration burst that
recommended by the controller");

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

[Keith Busch]

On Wed, Jun 24, 2020 at 09:34:08AM +0800, Baolin Wang wrote:
> OK, I understaood your concern. Now we will select the RR arbitration as default
> in nvme_enable_ctrl(), but for some cases, we will not set the arbitration burst
> values from userspace, and we still want to use the defaut arbitration burst that
> recommended by the controller, taking into consideration any latency requirements.
> 
> So I'd like to add a parameter to decide if we can use the default arbitration
> burst like below, how do yo think? Thanks.

I wouldn't call this the 'default' arbitration since it usually is not
the default. This is the 'recommended' arbitration value.
 
> static bool use_default_arb;
> module_param(use_default_arb, bool, 0444);
> MODULE_PARM_DESC(use_default_arb, "use default arbitration burst that
> recommended by the controller");

"use controller's recommended arbitration burst"

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

[Baolin Wang]

On Tue, Jun 23, 2020 at 07:51:35PM -0700, Keith Busch wrote:
> On Wed, Jun 24, 2020 at 09:34:08AM +0800, Baolin Wang wrote:
> > OK, I understaood your concern. Now we will select the RR arbitration as default
> > in nvme_enable_ctrl(), but for some cases, we will not set the arbitration burst
> > values from userspace, and we still want to use the defaut arbitration burst that
> > recommended by the controller, taking into consideration any latency requirements.
> > 
> > So I'd like to add a parameter to decide if we can use the default arbitration
> > burst like below, how do yo think? Thanks.
> 
> I wouldn't call this the 'default' arbitration since it usually is not
> the default. This is the 'recommended' arbitration value.

OK. Change to rename the variable as:
static bool use_recommended_arb;

>  
> > static bool use_default_arb;
> > module_param(use_default_arb, bool, 0444);
> > MODULE_PARM_DESC(use_default_arb, "use default arbitration burst that
> > recommended by the controller");
> 
> "use controller's recommended arbitration burst"

Sure. Thanks.

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

[Keith Busch]

On Tue, Jun 23, 2020 at 09:24:32PM +0800, Baolin Wang wrote:
> +void nvme_set_arbitration_burst(struct nvme_ctrl *ctrl)
> +{
> +	u32 result;
> +	int status;
> +
> +	if (!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,

Since 'rab' is an 8-bit field, but the feature's AB value is only 3
bits, we should add a validity check.

> +}
> +EXPORT_SYMBOL_GPL(nvme_set_arbitration_burst);

I don't see any particular reason to export this function just for the
pci transport to use. Just make it a local static function an call it
from nvme_init_identify().

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

[Baolin Wang]

On Tue, Jun 23, 2020 at 07:57:15PM -0700, Keith Busch wrote:
> On Tue, Jun 23, 2020 at 09:24:32PM +0800, Baolin Wang wrote:
> > +void nvme_set_arbitration_burst(struct nvme_ctrl *ctrl)
> > +{
> > +	u32 result;
> > +	int status;
> > +
> > +	if (!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,
> 
> Since 'rab' is an 8-bit field, but the feature's AB value is only 3
> bits, we should add a validity check.

Sure.

> 
> > +}
> > +EXPORT_SYMBOL_GPL(nvme_set_arbitration_burst);
> 
> I don't see any particular reason to export this function just for the
> pci transport to use. Just make it a local static function an call it
> from nvme_init_identify().

OK. Thanks.

[PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Baolin Wang]

Introduce a new capability macro to indicate if the controller
supports the memory buffer or not, instead of reading the
NVME_REG_CMBSZ register.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4b4b7b7..00c2701 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1760,7 +1760,7 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	int bar;
 
-	if (dev->cmb_size)
+	if (!NVME_CAP_CMBS(dev->ctrl.cap) || dev->cmb_size)
 		return;
 
 	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 5ce51ab..e3726be 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -134,6 +134,7 @@ enum {
 #define NVME_CAP_NSSRC(cap)	(((cap) >> 36) & 0x1)
 #define NVME_CAP_MPSMIN(cap)	(((cap) >> 48) & 0xf)
 #define NVME_CAP_MPSMAX(cap)	(((cap) >> 52) & 0xf)
+#define NVME_CAP_CMBS(cap)	(((cap) >> 57) & 0x1)
 
 #define NVME_CMB_BIR(cmbloc)	((cmbloc) & 0x7)
 #define NVME_CMB_OFST(cmbloc)	(((cmbloc) >> 12) & 0xfffff)
-- 
1.8.3.1

Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Christoph Hellwig]

On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote:
> Introduce a new capability macro to indicate if the controller
> supports the memory buffer or not, instead of reading the
> NVME_REG_CMBSZ register.

This is a complex issue.  The CMBS bit was only added in NVMe 1.4 as
a backwards incompatible change, as the CMB addressing scheme can lead
to data corruption.  The CMBS was added as part of the horribe hack
that also involves the CBA field, which we'll need to see before
using it to work around the addressing issue.  At the same time we
should also continue supporting the legacy pre-1.4 CMB with a warning
(and may reject it if we know we run in a VM).

Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Baolin Wang]

On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote:
> > Introduce a new capability macro to indicate if the controller
> > supports the memory buffer or not, instead of reading the
> > NVME_REG_CMBSZ register.
> 
> This is a complex issue.  The CMBS bit was only added in NVMe 1.4 as
> a backwards incompatible change, as the CMB addressing scheme can lead

Ah, right, I think I should add another version condition:
if ((ctrl->vs >= NVME_VS(1, 4, 0) && !NVME_CAP_CMBS(dev->ctrl.cap)) ||
    dev->cmb_size)
    return;

> to data corruption.  The CMBS was added as part of the horribe hack
> that also involves the CBA field, which we'll need to see before
> using it to work around the addressing issue.  At the same time we
> should also continue supporting the legacy pre-1.4 CMB with a warning
> (and may reject it if we know we run in a VM).

Thanks for your explanation. I saw we've added an CMB sysfs attribute
to get the CMB location and size if we enabled CMB, so should we still
add some information in nvme_map_cmb() let user know explicitly?

dev_info(dev->ctrl.device, "Registered the CMB, bar=0x%x, size=%lld\n", bar,
size);

Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Baolin Wang]

> On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote:
> > > Introduce a new capability macro to indicate if the controller
> > > supports the memory buffer or not, instead of reading the
> > > NVME_REG_CMBSZ register.
> > 
> > This is a complex issue.  The CMBS bit was only added in NVMe 1.4 as
> > a backwards incompatible change, as the CMB addressing scheme can lead
> 
> Ah, right, I think I should add another version condition:
> if ((ctrl->vs >= NVME_VS(1, 4, 0) && !NVME_CAP_CMBS(dev->ctrl.cap)) ||
>     dev->cmb_size)
>    return;

After more thinking, now we should read NVME_REG_VS register to get the controller version
when using the CMBS bit in Capabilities register, there is no benefit, so I'll drop this patch. Thanks.

> > to data corruption.  The CMBS was added as part of the horribe hack
> > that also involves the CBA field, which we'll need to see before
> > using it to work around the addressing issue.  At the same time we
> > should also continue supporting the legacy pre-1.4 CMB with a warning
> > (and may reject it if we know we run in a VM).

Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Keith Busch]

On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote:
> > Introduce a new capability macro to indicate if the controller
> > supports the memory buffer or not, instead of reading the
> > NVME_REG_CMBSZ register.
> 
> This is a complex issue.  The CMBS bit was only added in NVMe 1.4 as
> a backwards incompatible change, as the CMB addressing scheme can lead
> to data corruption.  The CMBS was added as part of the horribe hack
> that also involves the CBA field, which we'll need to see before
> using it to work around the addressing issue.  At the same time we
> should also continue supporting the legacy pre-1.4 CMB with a warning
> (and may reject it if we know we run in a VM).

Well, a CMB from an emulated controller (like qemu's) can be used within
a VM. It's only if you direct assign a PCI function that CMB usage
breaks.

Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro

[Christoph Hellwig]

On Tue, Jun 23, 2020 at 07:58:17PM -0700, Keith Busch wrote:
> On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote:
> > > Introduce a new capability macro to indicate if the controller
> > > supports the memory buffer or not, instead of reading the
> > > NVME_REG_CMBSZ register.
> > 
> > This is a complex issue.  The CMBS bit was only added in NVMe 1.4 as
> > a backwards incompatible change, as the CMB addressing scheme can lead
> > to data corruption.  The CMBS was added as part of the horribe hack
> > that also involves the CBA field, which we'll need to see before
> > using it to work around the addressing issue.  At the same time we
> > should also continue supporting the legacy pre-1.4 CMB with a warning
> > (and may reject it if we know we run in a VM).
> 
> Well, a CMB from an emulated controller (like qemu's) can be used within
> a VM. It's only if you direct assign a PCI function that CMB usage
> breaks.

But we have no idea if a controller is assigned or emulated.

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

[Baolin Wang]

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

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 c3a60d8..0eca820 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2963,7 +2963,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 3/3] nvme: Use USEC_PER_SEC instead of magic numbers

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>