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

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).

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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);


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Sagi Grimberg]

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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");


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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"

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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().

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme