Re: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

Re: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

[Roland Dreier]

The ib_addr module depends on CONFIG_INET, because it uses symbols
like arp_tbl, which are only exported if INET is enabled.

I fixed this up by creating a new (non-user-visible) config symbol to
control when ib_addr is built -- I put the following diff on top of
your patch in my tree:

diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index bdf0891..48c8bb5 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -29,6 +29,11 @@ config INFINIBAND_USER_ACCESS
 	  libibverbs, libibcm and a hardware driver library from
 	  <http://www.openib.org>.
 
+config INFINIBAND_ADDR_TRANS
+	tristate
+	depends on INFINIBAND && INET
+	default y
+
 source "drivers/infiniband/hw/mthca/Kconfig"
 
 source "drivers/infiniband/ulp/ipoib/Kconfig"
diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
index 2393e9d..935851d 100644
--- a/drivers/infiniband/core/Makefile
+++ b/drivers/infiniband/core/Makefile
@@ -1,7 +1,8 @@
 obj-$(CONFIG_INFINIBAND) +=		ib_core.o ib_mad.o ib_sa.o \
-					ib_cm.o ib_addr.o
+					ib_cm.o 
 obj-$(CONFIG_INFINIBAND_USER_MAD) +=	ib_umad.o
 obj-$(CONFIG_INFINIBAND_USER_ACCESS) +=	ib_uverbs.o ib_ucm.o
+obj-$(CONFIG_INFINIBAND_ADDR_TRANS) +=	ib_addr.o
 
 ib_core-y :=			packer.o ud_header.o verbs.o sysfs.o \
 				device.o fmr_pool.o cache.o

RE: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

[Sean Hefty]

>The ib_addr module depends on CONFIG_INET, because it uses symbols
>like arp_tbl, which are only exported if INET is enabled.
>
>I fixed this up by creating a new (non-user-visible) config symbol to
>control when ib_addr is built -- I put the following diff on top of
>your patch in my tree:

Thanks!
-Sean

Re: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

[Roland Dreier]

 > +struct workqueue_struct *rdma_wq;
 > +EXPORT_SYMBOL(rdma_wq);

Sean, I don't think I saw an answer when I asked you this before.  Why
is ib_addr exporting a workqueue?  Is there some sort of ordering
constraint that is forcing other modules to go through the same
workqueue for things?

This seems like a very fragile internal thing to be exposing, and I'm
wondering if there's a better way to handle it.

 - R.

Re: Re: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

[Sean Hefty]

Roland Dreier wrote:
>  > +struct workqueue_struct *rdma_wq;
>  > +EXPORT_SYMBOL(rdma_wq);
> 
> Sean, I don't think I saw an answer when I asked you this before.  Why
> is ib_addr exporting a workqueue?  Is there some sort of ordering
> constraint that is forcing other modules to go through the same
> workqueue for things?
> 
> This seems like a very fragile internal thing to be exposing, and I'm
> wondering if there's a better way to handle it.

I responded in a different thread, but here's what I wrote:

"This is simply an attempt to reduce/combine work queues used by the Infiniband 
code.  This keeps the threading a little simpler in the rdma_cm, since all 
callbacks are invoked using the same work queue.  (I'm also using this with the 
local SA/multicast code, but that's not ready for merging.)"

There's no specific ordering constraint that's required.  We're just ending up 
with several Infiniband modules creating their own work queues (ib_mad, ib_cm, 
ib_addr, rdma_cm, plus a couple more in modules under development), and this is 
an attempt to reduce that.  If having separate work queues would work better, 
there shouldn't be anything that prevents this.

- Sean

Re: Re: [PATCH 4/6 v2] IB: address translation to map IP toIB addresses (GIDs)

[Roland Dreier]

    Sean> "This is simply an attempt to reduce/combine work queues
    Sean> used by the Infiniband code.  This keeps the threading a
    Sean> little simpler in the rdma_cm, since all callbacks are
    Sean> invoked using the same work queue.  (I'm also using this
    Sean> with the local SA/multicast code, but that's not ready for
    Sean> merging.)"

How does it keep the threading model simpler?  Is this an inter-module
dependency.

    Sean> There's no specific ordering constraint that's required.
    Sean> We're just ending up with several Infiniband modules
    Sean> creating their own work queues (ib_mad, ib_cm, ib_addr,
    Sean> rdma_cm, plus a couple more in modules under development),
    Sean> and this is an attempt to reduce that.  If having separate
    Sean> work queues would work better, there shouldn't be anything
    Sean> that prevents this.

It seems like it would be cleaner for each module to have its own
workqueue if it needs one.  There's also schedule_work(), although
that goes to a multi-threaded workqueue.  Michael Tsirkin has
suggested creating a system-wide single-threaded workqueue (ie
something like schedule_ordered_work()) for everyone that occasionally
needs a single-threaded workqueue.

 - R.