Skip to contentSkip to conversation

nvme: add virt boundary per ctrl

PATCH New Max Gurtovoy 4 participants 12 messages Aug 16, 2017 – Aug 22, 2017

Subsystems (3)
NVMe NVMe target NVMe FC transport
lore
Share & tools
lore

Max Gurtovoy

author
#0 lore raw

PATCH 1/3 nvme: add virt boundary per ctrl

Prepare an infrastructure for controllers that can handle arbitrarily sized bios (e.g advanced RDMA and PCI ctrls). Add virt_boundary_mask according to each ctrl constraints.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c   |    2 +-
 drivers/nvme/host/fc.c     |    1 +
 drivers/nvme/host/nvme.h   |    1 +
 drivers/nvme/host/pci.c    |    1 +
 drivers/nvme/host/rdma.c   |    1 +
 drivers/nvme/target/loop.c |    1 +
 6 files changed, 6 insertions(+), 1 deletions(-)
drivers/nvme/host/core.c +1 -1
index 37046ac..8cf37e7 100644
@@ -1503,7 +1503,7 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
 	}
 	if (ctrl->quirks & NVME_QUIRK_STRIPE_SIZE)
 		blk_queue_chunk_sectors(q, ctrl->max_hw_sectors);
-	blk_queue_virt_boundary(q, ctrl->page_size - 1);
+	blk_queue_virt_boundary(q, ctrl->virt_boundary_mask);
 	if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
 		vwc = true;
 	blk_queue_write_cache(q, vwc, vwc);
drivers/nvme/host/fc.c +1 -0
index 5c2a08e..707db4d 100644
@@ -2354,6 +2354,7 @@ enum {
 	segs = min_t(u32, NVME_FC_MAX_SEGMENTS,
 			ctrl->lport->ops->max_sgl_segments);
 	ctrl->ctrl.max_hw_sectors = (segs - 1) << (PAGE_SHIFT - 9);
+	ctrl->ctrl.virt_boundary_mask = ctrl->ctrl.page_size - 1;
 
 	ret = nvme_init_identify(&ctrl->ctrl);
 	if (ret)
drivers/nvme/host/nvme.h +1 -0
index 8f2a168..803c211 100644
@@ -167,6 +167,7 @@ struct nvme_ctrl {
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
+	unsigned long virt_boundary_mask;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
drivers/nvme/host/pci.c +1 -0
index 74a124a..da4e130 100644
@@ -2140,6 +2140,7 @@ static void nvme_reset_work(struct work_struct *work)
 	if (result)
 		goto out;
 
+	dev->ctrl.virt_boundary_mask = dev->ctrl.page_size - 1;
 	result = nvme_init_identify(&dev->ctrl);
 	if (result)
 		goto out;
drivers/nvme/host/rdma.c +1 -0
index da04df1..04a0d33 100644
@@ -1584,6 +1584,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)
 
 	ctrl->ctrl.max_hw_sectors =
 		(ctrl->max_fr_pages - 1) << (PAGE_SHIFT - 9);
+	ctrl->ctrl.virt_boundary_mask = ctrl->ctrl.page_size - 1;
 
 	error = nvme_init_identify(&ctrl->ctrl);
 	if (error)
drivers/nvme/target/loop.c +1 -0
index 717ed7d..b0a831d 100644
@@ -402,6 +402,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 
 	ctrl->ctrl.max_hw_sectors =
 		(NVME_LOOP_MAX_SEGMENTS - 1) << (PAGE_SHIFT - 9);
+	ctrl->ctrl.virt_boundary_mask = ctrl->ctrl.page_size - 1;
 
 	error = nvme_init_identify(&ctrl->ctrl);
 	if (error)
Signature · 1 line
1.7.1
author
#1 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Each ctrl will limit max_segments according to it's hardware restrictions. Implement the restriction for RDMA transport.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c |    4 ++--
 drivers/nvme/host/nvme.h |    1 +
 drivers/nvme/host/rdma.c |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +2 -2
index 8cf37e7..cc0681e 100644
@@ -1495,8 +1495,8 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
 	bool vwc = false;
 
 	if (ctrl->max_hw_sectors) {
-		u32 max_segments =
-			(ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1;
+		u32 max_segments = min_not_zero(ctrl->max_segments,
+			(ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1);
 
 		blk_queue_max_hw_sectors(q, ctrl->max_hw_sectors);
 		blk_queue_max_segments(q, min_t(u32, max_segments, USHRT_MAX));
drivers/nvme/host/nvme.h +1 -0
index 803c211..0d6a91a 100644
@@ -147,6 +147,7 @@ struct nvme_ctrl {
 	u64 cap;
 	u32 page_size;
 	u32 max_hw_sectors;
+	u32 max_segments;
 	u16 oncs;
 	u16 vid;
 	u16 oacs;
drivers/nvme/host/rdma.c +1 -0
index 04a0d33..cf71895 100644
@@ -1582,6 +1582,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)
 	if (error)
 		goto out_cleanup_queue;
 
+	ctrl->ctrl.max_segments = ctrl->max_fr_pages;
 	ctrl->ctrl.max_hw_sectors =
 		(ctrl->max_fr_pages - 1) << (PAGE_SHIFT - 9);
 	ctrl->ctrl.virt_boundary_mask = ctrl->ctrl.page_size - 1;
Signature · 1 line
1.7.1
maintainer
#2 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 16 lines

Each ctrl will limit max_segments according to it's hardware restrictions. Implement the restriction for RDMA transport.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
  drivers/nvme/host/core.c |    4 ++--
  drivers/nvme/host/nvme.h |    1 +
  drivers/nvme/host/rdma.c |    1 +
  3 files changed, 4 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +0 -0
index 8cf37e7..cc0681e 100644

drivers/nvme/host/core.c static void nvme_set_queue_limits(struct nvme_ctrl *ctrl, · line 1499

Quoted 7 lines
drivers/nvme/host/core.c +2 -2
@@ -1495,8 +1495,8 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
  	bool vwc = false;
 
  	if (ctrl->max_hw_sectors) {
-		u32 max_segments =
-			(ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1;
+		u32 max_segments = min_not_zero(ctrl->max_segments,
+			(ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1);

Why is this needed with patches 1,3 applied?

We set the page_size and we set max_hw_sectors accordingly.

author
#3 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 26 lines · Sagi Grimberg
Quoted 16 lines

Each ctrl will limit max_segments according to it's hardware restrictions. Implement the restriction for RDMA transport.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
  drivers/nvme/host/core.c |    4 ++--
  drivers/nvme/host/nvme.h |    1 +
  drivers/nvme/host/rdma.c |    1 +
  3 files changed, 4 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +0 -0
index 8cf37e7..cc0681e 100644

drivers/nvme/host/core.c static void nvme_set_queue_limits(struct · line 1499

Quoted 7 lines
drivers/nvme/host/core.c +2 -2
@@ -1495,8 +1495,8 @@ static void nvme_set_queue_limits(struct
 nvme_ctrl *ctrl,
      bool vwc = false;
        if (ctrl->max_hw_sectors) {
-        u32 max_segments =
-            (ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1;
+        u32 max_segments = min_not_zero(ctrl->max_segments,
+            (ctrl->max_hw_sectors / (ctrl->page_size >> 9)) + 1);

Why is this needed with patches 1,3 applied?

Quoted 1 line

We set the page_size and we set max_hw_sectors accordingly.

We didn't touch the ctrl->page_size. It's always 4k (the target checks it). This may cause max_segments to be bigger than hw limitation (in PPC for example).

maintainer
#4 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 8 lines
Quoted 4 lines

Why is this needed with patches 1,3 applied?

We set the page_size and we set max_hw_sectors accordingly.

We didn't touch the ctrl->page_size. It's always 4k (the target checks it). This may cause max_segments to be bigger than hw limitation (in PPC for example).

Right, got confused for a second, but why shouldn't we modify the ctrl page_size in rdma? Its not really a controller attribute, but rather the local HCA attribute really.

Or, we could simply fix nvme-core to take PAGE_SIZE if supported by the ctrl and fallback to 4k otherwise.

maintainer
#5 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 5 lines · Sagi Grimberg
Quoted 5 lines
Quoted 4 lines

Why is this needed with patches 1,3 applied?

We set the page_size and we set max_hw_sectors accordingly.

Quoted 7 lines
Quoted 3 lines

We didn't touch the ctrl->page_size. It's always 4k (the target checks it). This may cause max_segments to be bigger than hw limitation (in PPC for example).

Right, got confused for a second, but why shouldn't we modify the ctrl page_size in rdma? Its not really a controller attribute, but rather the local HCA attribute really.

But the controller needs to support it, and NVMe controllers are only required to support a 4k page size, the rest is optional.

Quoted 2 lines

Or, we could simply fix nvme-core to take PAGE_SIZE if supported by the ctrl and fallback to 4k otherwise.

For PCIe (before fabrics existed) we used to do that, but we changed it for ppc64 in this commit:

c5c9f25b98 ("NVMe: default to 4k device page size")

maintainer
#6 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

Why is this needed with patches 1,3 applied?

We set the page_size and we set max_hw_sectors accordingly.

We didn't touch the ctrl->page_size. It's always 4k (the target checks it). This may cause max_segments to be bigger than hw limitation (in PPC for example).

Quoted 6 lines
Quoted 3 lines

Right, got confused for a second, but why shouldn't we modify the ctrl page_size in rdma? Its not really a controller attribute, but rather the local HCA attribute really.

But the controller needs to support it, and NVMe controllers are only required to support a 4k page size, the rest is optional.

How exacly is the controller required to support the pag_size for fabrics?

It could have implications on mdts, but AFAIR, mdts is reported in units of MPSMIN so I'm not exactly sure how exactly the controller needs to support it.

Quoted 7 lines
Quoted 2 lines

Or, we could simply fix nvme-core to take PAGE_SIZE if supported by the ctrl and fallback to 4k otherwise.

For PCIe (before fabrics existed) we used to do that, but we changed it for ppc64 in this commit:

c5c9f25b98 ("NVMe: default to 4k device page size")

Yea... I'm wandering if this is specific to NVMe or it can trigger in RDMA as well.

Note that for iSER we always use 4k page vectors, but that was historically done before we had gaps detection in the block layer and we had higher chance to do bounce buffering in iSER.

maintainer
#7 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 5 lines · Sagi Grimberg
Quoted 2 lines

But the controller needs to support it, and NVMe controllers are only required to support a 4k page size, the rest is optional.

How exacly is the controller required to support the pag_size for fabrics?

It doesn't matter at all, except as the scaling factor for a few reported values.

Quoted 7 lines
Quoted 4 lines

For PCIe (before fabrics existed) we used to do that, but we changed it for ppc64 in this commit:

c5c9f25b98 ("NVMe: default to 4k device page size")

Yea... I'm wandering if this is specific to NVMe or it can trigger in RDMA as well.

As far as I can tell we could hit it with RDMA under the same circumstances (weird iommu with a smaller page size than the system page size), but for RDMA the nvme controller page size isn't the one that matter, but rather the MR page size.

So for now to be safe it might be a good idea to always use a 4k MR page size..

author
#8 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 3 lines

So for now to be safe it might be a good idea to always use a 4k MR page size..

So should I send a fix only for the MR page size (4k) for now ? Or should we add the infrastructure for virt_boundary and max_segments per ctrl too ?

maintainer
#9 lore raw

PATCH 2/3 nvme: introduce max_segments controller attribute

Quoted 7 lines · Max Gurtovoy
Quoted 3 lines

So for now to be safe it might be a good idea to always use a 4k MR page size..

So should I send a fix only for the MR page size (4k) for now ? Or should we add the infrastructure for virt_boundary and max_segments per ctrl too ?

We'll eventually want both, but for 4.13 fixing the MR page size to 4k should be fine.

author
#10 lore raw

PATCH 3/3 nvme-rdma: fix virtual boundary calculation

In reply to Max Gurtovoy

Set the virtual boundary to correspond the mapping restrictions of the HCA. This fixes the mismatch in systems with page size > 4KiB.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/rdma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/nvme/host/rdma.c +1 -1
index cf71895..8ab5a71 100644
@@ -1585,7 +1585,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)
 	ctrl->ctrl.max_segments = ctrl->max_fr_pages;
 	ctrl->ctrl.max_hw_sectors =
 		(ctrl->max_fr_pages - 1) << (PAGE_SHIFT - 9);
-	ctrl->ctrl.virt_boundary_mask = ctrl->ctrl.page_size - 1;
+	ctrl->ctrl.virt_boundary_mask = PAGE_SIZE - 1;
 
 	error = nvme_init_identify(&ctrl->ctrl);
 	if (error)
Signature · 1 line
1.7.1
#11 lore raw

PATCH 1/3 nvme: add virt boundary per ctrl

In reply to Max Gurtovoy
Quoted 5 lines · Max Gurtovoy

Prepare an infrastructure for controllers that can handle arbitrarily sized bios (e.g advanced RDMA and PCI ctrls). Add virt_boundary_mask according to each ctrl constraints.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
Quoted 10 lines
---
  drivers/nvme/host/core.c   |    2 +-
  drivers/nvme/host/fc.c     |    1 +
  drivers/nvme/host/nvme.h   |    1 +
  drivers/nvme/host/pci.c    |    1 +
  drivers/nvme/host/rdma.c   |    1 +
  drivers/nvme/target/loop.c |    1 +
  6 files changed, 6 insertions(+), 1 deletions(-)

Looks fine to me.

Reviewed-by James Smart <james.smart at broadcom.com>