Skip to contentSkip to conversation

IB: expand ib_umem_get() prototype

PATCH In review [email protected] 2 participants 3 messages Apr 1, 2008 – Apr 8, 2008

Subsystems (2)
InfiniBand Mellanox MLX4 IB driver
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH v6 3/3 IB: expand ib_umem_get() prototype

Add a new parameter, dmasync, to the ib_umem_get() prototype. Use dmasync = 1 when mapping user-allocated CQs with ib_umem_get().

Signed-off-by Arthur Kepner <[email protected]>

---

 drivers/infiniband/core/umem.c               |   17 +++++++++----
 drivers/infiniband/hw/amso1100/c2_provider.c |    2 -
 drivers/infiniband/hw/cxgb3/iwch_provider.c  |    2 -
 drivers/infiniband/hw/ehca/ehca_mrmw.c       |    2 -
 drivers/infiniband/hw/ipath/ipath_mr.c       |    3 +-
 drivers/infiniband/hw/mlx4/cq.c              |    2 -
 drivers/infiniband/hw/mlx4/doorbell.c        |    2 -
 drivers/infiniband/hw/mlx4/mr.c              |    3 +-
 drivers/infiniband/hw/mlx4/qp.c              |    2 -
 drivers/infiniband/hw/mlx4/srq.c             |    2 -
 drivers/infiniband/hw/mthca/mthca_provider.c |    8 +++++-
 drivers/infiniband/hw/mthca/mthca_user.h     |   10 +++++++-
 drivers/infiniband/hw/nes/nes_verbs.c        |    2 -
 include/rdma/ib_umem.h                       |    4 +--
 include/rdma/ib_verbs.h                      |   33 +++++++++++++++++++++++++++
 15 files changed, 75 insertions(+), 19 deletions(-)
drivers/infiniband/core/umem.c +12 -5
index 4e3128f..fe78f7d 100644
@@ -38,6 +38,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/sched.h>
 #include <linux/hugetlb.h>
+#include <linux/dma-attrs.h>
 
 #include "uverbs.h"
 

@@ -72,9 +73,10 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
  * @addr: userspace virtual address to start at
  * @size: length of region to pin
  * @access: IB_ACCESS_xxx flags for memory being pinned
+ * @dmasync: flush in-flight DMA when the memory region is written
  */
 struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
-			    size_t size, int access)
+			    size_t size, int access, int dmasync)
 {
 	struct ib_umem *umem;
 	struct page **page_list;

@@ -87,6 +89,10 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 	int ret;
 	int off;
 	int i;
+	DEFINE_DMA_ATTRS(attrs);
+
+	if (dmasync)
+		dma_set_attr(DMA_ATTR_WRITE_BARRIER, &attrs);
 
 	if (!can_do_mlock())
 		return ERR_PTR(-EPERM);

@@ -174,10 +180,11 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 				sg_set_page(&chunk->page_list[i], page_list[i + off], PAGE_SIZE, 0);
 			}
 
-			chunk->nmap = ib_dma_map_sg(context->device,
-						    &chunk->page_list[0],
-						    chunk->nents,
-						    DMA_BIDIRECTIONAL);
+			chunk->nmap = ib_dma_map_sg_attrs(context->device,
+							  &chunk->page_list[0],
+							  chunk->nents,
+							  DMA_BIDIRECTIONAL,
+							  &attrs);
 			if (chunk->nmap <= 0) {
 				for (i = 0; i < chunk->nents; ++i)
 					put_page(sg_page(&chunk->page_list[i]));
drivers/infiniband/hw/amso1100/c2_provider.c +1 -1
index 7a6cece..f571dff 100644
@@ -449,7 +449,7 @@ static struct ib_mr *c2_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 		return ERR_PTR(-ENOMEM);
 	c2mr->pd = c2pd;
 
-	c2mr->umem = ib_umem_get(pd->uobject->context, start, length, acc);
+	c2mr->umem = ib_umem_get(pd->uobject->context, start, length, acc, 0);
 	if (IS_ERR(c2mr->umem)) {
 		err = PTR_ERR(c2mr->umem);
 		kfree(c2mr);
drivers/infiniband/hw/cxgb3/iwch_provider.c +1 -1
index b2ea921..e0d0498 100644
@@ -602,7 +602,7 @@ static struct ib_mr *iwch_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	if (!mhp)
 		return ERR_PTR(-ENOMEM);
 
-	mhp->umem = ib_umem_get(pd->uobject->context, start, length, acc);
+	mhp->umem = ib_umem_get(pd->uobject->context, start, length, acc, 0);
 	if (IS_ERR(mhp->umem)) {
 		err = PTR_ERR(mhp->umem);
 		kfree(mhp);
drivers/infiniband/hw/ehca/ehca_mrmw.c +1 -1
index e239bbf..62a382c 100644
@@ -325,7 +325,7 @@ struct ib_mr *ehca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	}
 
 	e_mr->umem = ib_umem_get(pd->uobject->context, start, length,
-				 mr_access_flags);
+				 mr_access_flags, 0);
 	if (IS_ERR(e_mr->umem)) {
 		ib_mr = (void *)e_mr->umem;
 		goto reg_user_mr_exit1;
drivers/infiniband/hw/ipath/ipath_mr.c +2 -1
index db4ba92..9d343b7 100644
@@ -195,7 +195,8 @@ struct ib_mr *ipath_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 		goto bail;
 	}
 
-	umem = ib_umem_get(pd->uobject->context, start, length, mr_access_flags);
+	umem = ib_umem_get(pd->uobject->context, start, length,
+			   mr_access_flags, 0);
 	if (IS_ERR(umem))
 		return (void *) umem;
drivers/infiniband/hw/mlx4/cq.c +1 -1
index 7360bba..0fe475d 100644
@@ -116,7 +116,7 @@ struct ib_cq *mlx4_ib_create_cq(struct ib_device *ibdev, int entries, int vector
 		}
 
 		cq->umem = ib_umem_get(context, ucmd.buf_addr, buf_size,
-				       IB_ACCESS_LOCAL_WRITE);
+				       IB_ACCESS_LOCAL_WRITE, 1);
 		if (IS_ERR(cq->umem)) {
 			err = PTR_ERR(cq->umem);
 			goto err_cq;
drivers/infiniband/hw/mlx4/doorbell.c +1 -1
index 1c36087..0afde2d 100644
@@ -181,7 +181,7 @@ int mlx4_ib_db_map_user(struct mlx4_ib_ucontext *context, unsigned long virt,
 	page->user_virt = (virt & PAGE_MASK);
 	page->refcnt    = 0;
 	page->umem      = ib_umem_get(&context->ibucontext, virt & PAGE_MASK,
-				      PAGE_SIZE, 0);
+				      PAGE_SIZE, 0, 0);
 	if (IS_ERR(page->umem)) {
 		err = PTR_ERR(page->umem);
 		kfree(page);
drivers/infiniband/hw/mlx4/mr.c +2 -1
index fe2c2e9..68e9248 100644
@@ -132,7 +132,8 @@ struct ib_mr *mlx4_ib_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	if (!mr)
 		return ERR_PTR(-ENOMEM);
 
-	mr->umem = ib_umem_get(pd->uobject->context, start, length, access_flags);
+	mr->umem = ib_umem_get(pd->uobject->context, start, length,
+			       access_flags, 0);
 	if (IS_ERR(mr->umem)) {
 		err = PTR_ERR(mr->umem);
 		goto err_free;
drivers/infiniband/hw/mlx4/qp.c +1 -1
index 958e205..9a28b4d 100644
@@ -479,7 +479,7 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd,
 			goto err;
 
 		qp->umem = ib_umem_get(pd->uobject->context, ucmd.buf_addr,
-				       qp->buf_size, 0);
+				       qp->buf_size, 0, 0);
 		if (IS_ERR(qp->umem)) {
 			err = PTR_ERR(qp->umem);
 			goto err;
drivers/infiniband/hw/mlx4/srq.c +1 -1
index beaa3b0..ea910e4 100644
@@ -109,7 +109,7 @@ struct ib_srq *mlx4_ib_create_srq(struct ib_pd *pd,
 		}
 
 		srq->umem = ib_umem_get(pd->uobject->context, ucmd.buf_addr,
-					buf_size, 0);
+					buf_size, 0, 0);
 		if (IS_ERR(srq->umem)) {
 			err = PTR_ERR(srq->umem);
 			goto err_srq;
drivers/infiniband/hw/mthca/mthca_provider.c +7 -1
index 9e491df..25175ef 100644
@@ -1003,17 +1003,23 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	struct mthca_dev *dev = to_mdev(pd->device);
 	struct ib_umem_chunk *chunk;
 	struct mthca_mr *mr;
+	struct mthca_reg_mr ucmd;
 	u64 *pages;
 	int shift, n, len;
 	int i, j, k;
 	int err = 0;
 	int write_mtt_size;
 
+	if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd))
+		return ERR_PTR(-EFAULT);
+
 	mr = kmalloc(sizeof *mr, GFP_KERNEL);
 	if (!mr)
 		return ERR_PTR(-ENOMEM);
 
-	mr->umem = ib_umem_get(pd->uobject->context, start, length, acc);
+	mr->umem = ib_umem_get(pd->uobject->context, start, length, acc,
+			       ucmd.mr_attrs & MTHCA_MR_DMASYNC);
+
 	if (IS_ERR(mr->umem)) {
 		err = PTR_ERR(mr->umem);
 		goto err;
drivers/infiniband/hw/mthca/mthca_user.h +9 -1
index 02cc0a7..f8cb3b6 100644
@@ -41,7 +41,7 @@
  * Increment this value if any changes that break userspace ABI
  * compatibility are made.
  */
-#define MTHCA_UVERBS_ABI_VERSION	1
+#define MTHCA_UVERBS_ABI_VERSION	2
 
 /*
  * Make sure that all structs defined in this file remain laid out so

@@ -61,6 +61,14 @@ struct mthca_alloc_pd_resp {
 	__u32 reserved;
 };
 
+struct mthca_reg_mr {
+	__u32 mr_attrs;
+#define MTHCA_MR_DMASYNC 0x1
+/* mark the memory region with a DMA attribute that causes
+ * in-flight DMA to be flushed when the region is written to */
+	__u32 reserved;
+};
+
 struct mthca_create_cq {
 	__u32 lkey;
 	__u32 pdn;
drivers/infiniband/hw/nes/nes_verbs.c +1 -1
index a651e9d..aeb4696 100644
@@ -2373,7 +2373,7 @@ static struct ib_mr *nes_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	u8 single_page = 1;
 	u8 stag_key;
 
-	region = ib_umem_get(pd->uobject->context, start, length, acc);
+	region = ib_umem_get(pd->uobject->context, start, length, acc, 0);
 	if (IS_ERR(region)) {
 		return (struct ib_mr *)region;
 	}
include/rdma/ib_umem.h +2 -2
index 2229842..9ee0d2e 100644
@@ -62,7 +62,7 @@ struct ib_umem_chunk {
 #ifdef CONFIG_INFINIBAND_USER_MEM
 
 struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
-			    size_t size, int access);
+			    size_t size, int access, int dmasync);
 void ib_umem_release(struct ib_umem *umem);
 int ib_umem_page_count(struct ib_umem *umem);
 

@@ -72,7 +72,7 @@ int ib_umem_page_count(struct ib_umem *umem);
 
 static inline struct ib_umem *ib_umem_get(struct ib_ucontext *context,
 					  unsigned long addr, size_t size,
-					  int access) {
+					  int access, int dmasync) {
 	return ERR_PTR(-EINVAL);
 }
 static inline void ib_umem_release(struct ib_umem *umem) { }
include/rdma/ib_verbs.h +33 -0
index 701e7b4..e932fa0 100644
@@ -1515,6 +1515,24 @@ static inline void ib_dma_unmap_single(struct ib_device *dev,
 		dma_unmap_single(dev->dma_device, addr, size, direction);
 }
 
+static inline u64 ib_dma_map_single_attrs(struct ib_device *dev,
+					  void *cpu_addr, size_t size,
+					  enum dma_data_direction direction,
+					  struct dma_attrs *attrs)
+{
+	return dma_map_single_attrs(dev->dma_device, cpu_addr, size,
+				    direction, attrs);
+}
+
+static inline void ib_dma_unmap_single_attrs(struct ib_device *dev,
+					     u64 addr, size_t size,
+					     enum dma_data_direction direction,
+					     struct dma_attrs *attrs)
+{
+	return dma_unmap_single_attrs(dev->dma_device, addr, size,
+				      direction, attrs);
+}
+
 /**
  * ib_dma_map_page - Map a physical page to DMA address
  * @dev: The device for which the dma_addr is to be created

@@ -1584,6 +1602,21 @@ static inline void ib_dma_unmap_sg(struct ib_device *dev,
 		dma_unmap_sg(dev->dma_device, sg, nents, direction);
 }
 
+static inline int ib_dma_map_sg_attrs(struct ib_device *dev,
+				      struct scatterlist *sg, int nents,
+				      enum dma_data_direction direction,
+				      struct dma_attrs *attrs)
+{
+	return dma_map_sg_attrs(dev->dma_device, sg, nents, direction, attrs);
+}
+
+static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
+					 struct scatterlist *sg, int nents,
+					 enum dma_data_direction direction,
+					 struct dma_attrs *attrs)
+{
+	dma_unmap_sg_attrs(dev->dma_device, sg, nents, direction, attrs);
+}
 /**
  * ib_sg_dma_address - Return the DMA address from a scatter/gather entry
  * @dev: The device for which the DMA addresses were created
#1 lore raw

All of this looks good to me, and I'm happy we are finally fixing this up. However, now that it is finally ready to merge, I started thinking about the libmthca changes required to handle this, and I realized that we can actually avoid this:

drivers/infiniband/hw/mthca/mthca_user.h line 44

Quoted 8 lines
drivers/infiniband/hw/mthca/mthca_user.h +1 -1
@@ -41,7 +41,7 @@
  * Increment this value if any changes that break userspace ABI
  * compatibility are made.
  */
-#define MTHCA_UVERBS_ABI_VERSION	1
+#define MTHCA_UVERBS_ABI_VERSION	2

Instead the mthca kernel driver can look at the size of the data passed in from userspace, and fall back to the old behavior if it's talking to old userspace.

I think this is a net win. The current patch gives the following:

old kernel, old userspace: current behavior (subtle failure on Altix) new kernel, old userspace: refuses to run (on all systems) old kernel, new userspace: current behavior (broken on Altix), new kernel, new userspace: works everywhere

with my proposed change, the only difference is:

new kernel, old userspace: current behavior (subtle failure on Altix),
plus print a warning telling people to update userspace.

and given that the vast majority of the systems where mthca is used are *not* Altix systems, it seems more friendly to leave them working across a kernel update.

Andrew: if Arthur agrees with this, please roll the patch below into the current IB patch you have as part of the dma attrs series.

Thanks,
Roland

drivers/infiniband/hw/mthca/mthca_provider.c +11 -1
index ca5bab0..d2e1429 100644
@@ -367,6 +367,8 @@ static struct ib_ucontext *mthca_alloc_ucontext(struct ib_device *ibdev,
 		return ERR_PTR(-EFAULT);
 	}
 
+	context->reg_mr_warned = 0;
+
 	return &context->ibucontext;
 }
 

@@ -1013,7 +1015,15 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
 	int err = 0;
 	int write_mtt_size;
 
-	if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd))
+	if (udata->inlen < sizeof ucm) {
+		if (!to_mucontext(pd->uobject->context)->reg_mr_warned) {
+			mthca_warn(dev, "Process %s did not pass in MR attrs\n",
+				   current->comm);
+			mthca_warn(dev, "  Update libmthca to fix this.\n");
+		}
+		++to_mucontext(pd->uobject->context)->reg_mr_warned;
+		ucmd.mr_attrs = 0;
+	} else if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd))
 		return ERR_PTR(-EFAULT);
 
 	mr = kmalloc(sizeof *mr, GFP_KERNEL);
drivers/infiniband/hw/mthca/mthca_provider.h +1 -0
index 262616c..934bf95 100644
@@ -67,6 +67,7 @@ struct mthca_ucontext {
 	struct ib_ucontext          ibucontext;
 	struct mthca_uar            uar;
 	struct mthca_user_db_table *db_tab;
+	int			    reg_mr_warned;
 };
 
 struct mthca_mtt;
drivers/infiniband/hw/mthca/mthca_user.h +6 -4
index f8cb3b6..e1262c9 100644
@@ -41,7 +41,7 @@
  * Increment this value if any changes that break userspace ABI
  * compatibility are made.
  */
-#define MTHCA_UVERBS_ABI_VERSION	2
+#define MTHCA_UVERBS_ABI_VERSION	1
 
 /*
  * Make sure that all structs defined in this file remain laid out so

@@ -62,10 +62,12 @@ struct mthca_alloc_pd_resp {
 };
 
 struct mthca_reg_mr {
+/*
+ * Mark the memory region with a DMA attribute that causes
+ * in-flight DMA to be flushed when the region is written to:
+ */
+#define MTHCA_MR_DMASYNC	0x1
 	__u32 mr_attrs;
-#define MTHCA_MR_DMASYNC 0x1
-/* mark the memory region with a DMA attribute that causes
- * in-flight DMA to be flushed when the region is written to */
 	__u32 reserved;
 };
author Acked-by
#2 lore raw
Quoted 7 lines · Roland Dreier

All of this looks good to me, and I'm happy we are finally fixing this up. However, now that it is finally ready to merge, I started thinking about the libmthca changes required to handle this, and I realized that we can actually avoid this:
.....
with my proposed change, the only difference is:

Quoted 7 lines

new kernel, old userspace: current behavior (subtle failure on Altix),
plus print a warning telling people to update userspace.
....

Andrew: if Arthur agrees with this, please roll the patch below into the current IB patch you have as part of the dma attrs series.

Sounds fine to me, Roland.

Acked-by Arthur Kepner <[email protected]>
Signature · 1 line
Arthur