[ewg] RE: [ofa-general] [PATCH 4/9] ib_core: Add RDMAoE SA support

[Eli Cohen]

On Tue, Jun 16, 2009 at 10:27:33AM -0700, Sean Hefty wrote:
> >+
> >+	w->member->multicast.rec.qkey = cpu_to_be32(0xc2c);
> 
> How can a user control this?  An app needs the same qkey for unicast traffic.

In RDMAoE, the qkey has a fixed well-known value, which will be
returned both by multicast and path queries.


> 
> >+	atomic_inc(&w->member->refcount);
> 
> This needs to be moved below...
I don't follow you - please explain.

> >+static struct ib_sa_multicast *
> >+eth_join_multicast(struct ib_sa_client *client,
> >+		   struct ib_device *device, u8 port_num,
> >+		   struct ib_sa_mcmember_rec *rec,
> >+		   ib_sa_comp_mask comp_mask, gfp_t gfp_mask,
> >+		   int (*callback)(int status,
> >+				   struct ib_sa_multicast *multicast),
> >+		   void *context)
> >+{
> >+	struct mcast_device *dev;
> >+	struct eth_work *w;
> >+	struct mcast_member *member;
> >+	int err;
> >+
> >+	dev = ib_get_client_data(device, &mcast_client);
> >+	if (!dev)
> >+		return ERR_PTR(-ENODEV);
> >+
> >+	member = kzalloc(sizeof *member, gfp_mask);
> >+	if (!member)
> >+		return ERR_PTR(-ENOMEM);
> >+
> >+	w = kzalloc(sizeof *w, gfp_mask);
> >+	if (!w) {
> >+		err = -ENOMEM;
> >+		goto out1;
> >+	}
> >+	w->member = member;
> >+	w->device = device;
> >+	w->port_num = port_num;
> >+
> >+	member->multicast.context = context;
> >+	member->multicast.callback = callback;
> >+	member->client = client;
> >+	member->multicast.rec.mgid = rec->mgid;
> >+	init_completion(&member->comp);
> >+	atomic_set(&member->refcount, 1);
> >+
> >+	ib_sa_client_get(client);
> >+	INIT_WORK(&w->work, eth_mcast_work_handler);
> >+	queue_work(mcast_wq, &w->work);
> >+
> >+	return &member->multicast;
> 
> The user could leave/destroy the multicast join request before the queued work
> item runs.  We need to hold an additional reference on the member until the work
> item completes.
Yes, thanks for catching this. I'll fix and resend.

> 
> There's substantial differences in functionality between an IB multicast group
> and the Ethernet group.  I would rather see the differences hidden by the
> rdma_cm, than the IB SA module.
> 

This is a question of transparency. The motivation is to enable
non-cma apps that use the ib_sa_join_multicast() API to work without
changes.

> >diff --git a/drivers/infiniband/core/sa_query.c
> >b/drivers/infiniband/core/sa_query.c
> >index 1865049..7bf9b5c 100644
> >--- a/drivers/infiniband/core/sa_query.c
> >+++ b/drivers/infiniband/core/sa_query.c
> >@@ -45,6 +45,7 @@
> 
> There's not a userspace interface into the sa_query module.  How will those apps
> work, or apps that send MADs on QPs other than QP1?

Currently, these apps won't work. Sending MADs directly on QPs other
than QP1 will not work.
However, we expect that a userpsace interface to the sa_query module
will be implemented in the future, which will forward queries to
the kernel module.
Naturally, most kernel ULPs and user-level apps will use these
standard interfaces instead of implementing MAD queries themselves.