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

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.

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

index b75d41e..6ff12d8 100644
@@ -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 {

index a64d0ba..98785ac 100644
@@ -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

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

Hi Christoph,

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.

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

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

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 just kicked the technical reflector to fix this restriction on CMB queues. Let's see how that plays out.

I like that idea. We could do some interesting things by letting users/udev tweak the CMB's uses.

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.