[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Jon Derrick]

This patch introduces a quirk to allow a controller to specify a
preferred minimum queue depth for queues mapped within the CMB.

Queues located in the CMB currently inherit a restriction that queues
must be aligned with respect to the device page size. This restriction
often doesn't make sense with respect to the CMB, so we should allow
devices to let queues be mapped unaligned within the CMB.

Additionally, CMB sizes may be too small for the normal number and depth
of queues in system memory, but the controller may be fast enough to
handle a reduced queue depth.

Specifying this feature implies that the controller can handle
unaligned queues mapped within the CMB, otherwise the controller may as
well have the aligned, larger queue depth queues. Specifying a preferred
minimum queue depth of 1 (shift value of 0) is not supported as it
is assumed some amount of queueing is required.

Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
---
Applies against jens/for-4.5/nvme
 drivers/nvme/host/nvme.h | 12 ++++++++++++
 drivers/nvme/host/pci.c  | 10 ++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index b75d41e..6ff12d8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -43,6 +43,18 @@ enum nvme_quirks {
 	 * specific Identify field.
 	 */
 	NVME_QUIRK_STRIPE_SIZE			= (1 << 0),
+	/*
+	 * The controller specifies a preferred minimum queue depth
+	 * for queues in the CMB. Specifying a preferred minimum queue
+	 * depth implies that the device's CMB supports queues being
+	 * mapped unaligned with respect to the device page size.
+	 *
+	 * Bits 3:1 is the preferred minimum queue depth as a power-of-2,
+	 * with '0' being the default of (device page size / entry size)
+	 */
+	#define NVME_QUIRK_MIN_QD_SHIFT(quirks)	(((quirks) & 0x7) << 1)
+	#define QUIRKS_TO_MIN_QD_SHIFT(quirks)	(((quirks) >> 1) & 0x7)
+
 };
 
 struct nvme_ctrl {
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a64d0ba..98785ac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1255,13 +1255,17 @@ static void nvme_disable_queue(struct nvme_dev *dev, int qid)
 static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,
 				int entry_size)
 {
+	struct nvme_ctrl *ctrl = &dev->ctrl;
 	int q_depth = dev->q_depth;
 	unsigned q_size_aligned = roundup(q_depth * entry_size,
 					  dev->ctrl.page_size);
+	int min_qd = 64;
 
 	if (q_size_aligned * nr_io_queues > dev->cmb_size) {
 		u64 mem_per_q = div_u64(dev->cmb_size, nr_io_queues);
-		mem_per_q = round_down(mem_per_q, dev->ctrl.page_size);
+		int ctrl_mqd_shift = QUIRKS_TO_MIN_QD_SHIFT(ctrl->quirks);
+		if (!ctrl_mqd_shift)
+			mem_per_q = round_down(mem_per_q, dev->ctrl.page_size);
 		q_depth = div_u64(mem_per_q, entry_size);
 
 		/*
@@ -1269,7 +1273,9 @@ static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,
 		 * would be better to map queues in system memory with the
 		 * original depth
 		 */
-		if (q_depth < 64)
+		if (ctrl_mqd_shift)
+			min_qd = 1 << ctrl_mqd_shift;
+		if (q_depth < min_qd)
 			return -ENOMEM;
 	}
 
-- 
2.5.0

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Christoph Hellwig]

Hi Jon,

this patch confuses me for ? few reasons:

 - please don't overload the quirks bitmap with actual values
 - why do we even need to set a size instead of using up the CMB
   size?
 - why do we even need to add a quirk?  I quick look at the spec
   doesn't seem to require us to align the submission queue size
 - this patchs adds a quirk, but no use of it so it's effectively just dead
   code

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Jon Derrick]

Hi Christoph,

>  - please don't overload the quirks bitmap with actual values
I agree it may not have been the best place for it, but I wasn't sure how/where it could go to be matched with specific devices. Certain devices may want 8, 16, or 32.

>  - why do we even need to set a size instead of using up the CMB
>    size?
Consider a device with 32kB allocated for CMB. The SQes alignment restriction (detailed below) requires us to align to the controller page size. If we consider an example of 4kB pages, that gives us 8 SQes in the CMB, each with 64 entries (assuming 64B-sized sq entries).

Or instead, we could have 32 queues, each with 1K alignment, and 16 entries (assuming the device is fast enough).

>  - why do we even need to add a quirk?  I quick look at the spec
>    doesn't seem to require us to align the submission queue size
>  - this patchs adds a quirk, but no use of it so it's effectively just dead
>    code
It is not that apparent. It inherits the restriction on the Create I/O Submission Queue command (NVM-Express 1.2a, 5.4, Figure 54).

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Keith Busch]

On Fri, Dec 11, 2015@01:25:46PM -0700, Jon Derrick wrote:
> Hi Christoph,
> 
> >  - please don't overload the quirks bitmap with actual values
> I agree it may not have been the best place for it, but I wasn't sure how/where it could go to be matched with specific devices. Certain devices may want 8, 16, or 32.

IMHO, I think these need to be tunable from sysfs. There are several
ideas on carving up CMB for various purposes, and don't think we can
let the driver make these policy decisions for the user.
 
> >  - why do we even need to add a quirk?  I quick look at the spec
> >    doesn't seem to require us to align the submission queue size
> >  - this patchs adds a quirk, but no use of it so it's effectively just dead
> >    code
> It is not that apparent. It inherits the restriction on the Create I/O Submission Queue command (NVM-Express 1.2a, 5.4, Figure 54).

I just kicked the technical reflector to fix this restriction on CMB
queues. Let's see how that plays out.

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Jon Derrick]

> > >  - please don't overload the quirks bitmap with actual values
> > I agree it may not have been the best place for it, but I wasn't sure how/where it could go to be matched with specific devices. Certain devices may want 8, 16, or 32.
> 
> IMHO, I think these need to be tunable from sysfs. There are several
> ideas on carving up CMB for various purposes, and don't think we can
> let the driver make these policy decisions for the user.
>  
I like that idea. We could do some interesting things by letting users/udev tweak the CMB's uses.

[PATCH] nvme: Allow controllers to specify a min queue depth for CMB

[Christoph Hellwig]

On Fri, Dec 11, 2015@01:25:46PM -0700, Jon Derrick wrote:
> >  - why do we even need to add a quirk?  I quick look at the spec
> >    doesn't seem to require us to align the submission queue size
> >  - this patchs adds a quirk, but no use of it so it's effectively just dead
> >    code
> It is not that apparent. It inherits the restriction on the Create I/O
> Submission Queue command (NVM-Express 1.2a, 5.4, Figure 54).

It only requires the start aligned, not the size.  But yes, that would
force the next one to be unaligned so to be useful we'd need to an unaligned
start as well.