Re: [PATCH 4/5] infiniband: add "dmabarrier" argument to ib_umem_get()

Previous thread: [PATCH 3/5] dma: document dma_flags_set_attr() by akepner on Tuesday, October 2, 2007 - 7:47 pm. (2 messages)

Next thread: [PATCH 5/5] mthca: allow setting "dmabarrier" on user-allocated memory by akepner on Tuesday, October 2, 2007 - 7:50 pm. (3 messages)
From: akepner
Date: Tuesday, October 2, 2007 - 7:49 pm

Pass a "dmabarrier" argument to ib_umem_get() and use the new 
argument to control setting the DMA_BARRIER_ATTR attribute on 
the memory that ib_umem_get() maps for DMA.

Signed-off-by: Arthur Kepner <akepner@sgi.com>

---

 drivers/infiniband/core/umem.c               |    8 ++++++--
 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       |    2 +-
 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 |    2 +-
 include/rdma/ib_umem.h                       |    4 ++--
 12 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 664d2fa..093b58d 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -69,9 +69,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
+ * @dmabarrier: set "dmabarrier" attribute on this memory
  */
 struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
-			    size_t size, int access)
+			    size_t size, int access, int dmabarrier)
 {
 	struct ib_umem *umem;
 	struct page **page_list;
@@ -83,6 +84,9 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 	int ret;
 	int off;
 	int i;
+	int flags = dmabarrier ? dma_flags_set_attr(DMA_BARRIER_ATTR, 
+						    DMA_BIDIRECTIONAL) : 
+				 DMA_BIDIRECTIONAL;
 
 	if (!can_do_mlock())
 		return ERR_PTR(-EPERM);
@@ -160,7 +164,7 @@ struct ib_umem ...
From: David Miller
Date: Tuesday, October 2, 2007 - 8:10 pm

From: akepner@sgi.com

Acked-by: David S. Miller <davem@davemloft.net>

However I'm a little unhappy with how IA64 achieves this.

The last argument for dma_map_foo() is an enum not an int,
every platform other than IA64 properly defines the last
argument as "enum dma_data_direction".  It can take one
of several distinct values, it is not a mask.

This hijacking of the DMA direction argument is hokey at
best, and at worst is type bypassing which is going to
explode subtly for someone in the future and result in
a long painful debugging session.

Adding another argument could be painful to do this cleanly, but at
least with inline functions and macros it could just evaluate to
nothing on platforms that don't need it.

Either that, or we should turn the thing into an integer "flags"
across the board and audit every DMA mapping implementation so that it
can handle multiple bits being set.  But that's really ugly and
invites mistakes as I detailed above.

-

From: akepner
Date: Wednesday, October 3, 2007 - 10:51 am

I don't dispute your point about abusing the enum here, it 
just seemed the least objectionable, and most expedient way 
to go.  But I'll add that ia64 isn't alone, x86_64 also uses 
an int for the final argument to its dma_map_* implementations.

-- 
Arthur

-

Previous thread: [PATCH 3/5] dma: document dma_flags_set_attr() by akepner on Tuesday, October 2, 2007 - 7:47 pm. (2 messages)

Next thread: [PATCH 5/5] mthca: allow setting "dmabarrier" on user-allocated memory by akepner on Tuesday, October 2, 2007 - 7:50 pm. (3 messages)