Skip to contentSkip to conversation

ib_core: IBoE CMA device binding

PATCH In review Eli Cohen 3 participants 12 messages Aug 26, 2010 – Sep 7, 2010 5 messages not in the archive InfiniBand

lore
Share & tools
lore

Eli Cohen

author
#0 lore raw

PATCH v10 2/12 ib_core: IBoE CMA device binding

Add support for IBoE device binding and IP --> GID resolution. Path resolving and multicast joining are implemented within cma.c by filling the responses and pushing the callbacks to the cma work queue. IP->GID resolution always yields IPv6 link local addresses - remote GIDs are derived from the destination MAC address of the remote port. Multicast GIDs are always mapped to multicast MACs as is done in IPv6. Some helper functions are added to ib_addr.h. IPv4 multicast is enabled by translating IPv4 multicast addresses to IPv6 multicast as described in
http://www.mail-archive.com/[email protected]/msg02134.html.

Signed-off-by Eli Cohen <eli-VPRAkNaXOzVS1MOuV/[email protected]>

---
Changes from v9:
return -ENODEV instead of -EINVAL when net device is not found.

 drivers/infiniband/core/cma.c      |  278 +++++++++++++++++++++++++++++++++--
 drivers/infiniband/core/sa_query.c |    5 +-
 drivers/infiniband/core/ucma.c     |   45 +++++-
 include/rdma/ib_addr.h             |   98 +++++++++++++-
 4 files changed, 402 insertions(+), 24 deletions(-)
drivers/infiniband/core/cma.c +262 -16
index b930b81..a90511b 100644
@@ -59,6 +59,7 @@ MODULE_LICENSE("Dual BSD/GPL");
 #define CMA_CM_RESPONSE_TIMEOUT 20
 #define CMA_MAX_CM_RETRIES 15
 #define CMA_CM_MRA_SETTING (IB_CM_MRA_FLAG_DELAY | 24)
+#define IBOE_PACKET_LIFETIME 18
 
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device);

@@ -157,6 +158,7 @@ struct cma_multicast {
 	struct list_head	list;
 	void			*context;
 	struct sockaddr_storage	addr;
+	struct kref		mcref;
 };
 
 struct cma_work {

@@ -173,6 +175,12 @@ struct cma_ndev_work {
 	struct rdma_cm_event	event;
 };
 
+struct iboe_mcast_work {
+	struct work_struct	 work;
+	struct rdma_id_private	*id;
+	struct cma_multicast	*mc;
+};
+
 union cma_ip_addr {
 	struct in6_addr ip6;
 	struct {

@@ -281,6 +289,8 @@ static void cma_attach_to_dev(struct rdma_id_private *id_priv,
 	atomic_inc(&cma_dev->refcount);
 	id_priv->cma_dev = cma_dev;
 	id_priv->id.device = cma_dev->device;
+	id_priv->id.route.addr.dev_addr.transport =
+		rdma_node_get_transport(cma_dev->device->node_type);
 	list_add_tail(&id_priv->list, &cma_dev->id_list);
 }
 

@@ -290,6 +300,14 @@ static inline void cma_deref_dev(struct cma_device *cma_dev)
 		complete(&cma_dev->comp);
 }
 
+static inline void release_mc(struct kref *kref)
+{
+	struct cma_multicast *mc = container_of(kref, struct cma_multicast, mcref);
+
+	kfree(mc->multicast.ib);
+	kfree(mc);
+}
+
 static void cma_detach_from_dev(struct rdma_id_private *id_priv)
 {
 	list_del(&id_priv->list);

@@ -330,15 +348,29 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv)
 	union ib_gid gid;
 	int ret = -ENODEV;
 
-	rdma_addr_get_sgid(dev_addr, &gid);
+	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
+		iboe_addr_get_sgid(dev_addr, &gid);
+		list_for_each_entry(cma_dev, &dev_list, list) {
+			ret = ib_find_cached_gid(cma_dev->device, &gid,
+						 &id_priv->id.port_num, NULL);
+			if (!ret)
+				goto out;
+		}
+	}
+
+	memcpy(&gid, dev_addr->src_dev_addr +
+	       rdma_addr_gid_offset(dev_addr), sizeof gid);
 	list_for_each_entry(cma_dev, &dev_list, list) {
 		ret = ib_find_cached_gid(cma_dev->device, &gid,
 					 &id_priv->id.port_num, NULL);
-		if (!ret) {
-			cma_attach_to_dev(id_priv, cma_dev);
+		if (!ret)
 			break;
-		}
 	}
+
+out:
+	if (!ret)
+		cma_attach_to_dev(id_priv, cma_dev);
+
 	return ret;
 }
 

@@ -556,10 +588,16 @@ static int cma_ib_init_qp_attr(struct rdma_id_private *id_priv,
 {
 	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
 	int ret;
+	u16 pkey;
+
+	if (rdma_port_get_link_layer(id_priv->id.device, id_priv->id.port_num) ==
+	    IB_LINK_LAYER_INFINIBAND)
+		pkey = ib_addr_get_pkey(dev_addr);
+	else
+		pkey = 0xffff;
 
 	ret = ib_find_cached_pkey(id_priv->id.device, id_priv->id.port_num,
-				  ib_addr_get_pkey(dev_addr),
-				  &qp_attr->pkey_index);
+				  pkey, &qp_attr->pkey_index);
 	if (ret)
 		return ret;
 

@@ -737,8 +775,8 @@ static inline int cma_user_data_offset(enum rdma_port_space ps)
 
 static void cma_cancel_route(struct rdma_id_private *id_priv)
 {
-	switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
-	case RDMA_TRANSPORT_IB:
+	switch (rdma_port_get_link_layer(id_priv->id.device, id_priv->id.port_num)) {
+	case IB_LINK_LAYER_INFINIBAND:
 		if (id_priv->query)
 			ib_sa_cancel_query(id_priv->query_id, id_priv->query);
 		break;

@@ -816,8 +854,17 @@ static void cma_leave_mc_groups(struct rdma_id_private *id_priv)
 		mc = container_of(id_priv->mc_list.next,
 				  struct cma_multicast, list);
 		list_del(&mc->list);
-		ib_sa_free_multicast(mc->multicast.ib);
-		kfree(mc);
+		switch (rdma_port_get_link_layer(id_priv->cma_dev->device, id_priv->id.port_num)) {
+		case IB_LINK_LAYER_INFINIBAND:
+			ib_sa_free_multicast(mc->multicast.ib);
+			kfree(mc);
+			break;
+		case IB_LINK_LAYER_ETHERNET:
+			kref_put(&mc->mcref, release_mc);
+			break;
+		default:
+			break;
+		}
 	}
 }
 

@@ -833,7 +880,7 @@ void rdma_destroy_id(struct rdma_cm_id *id)
 	mutex_lock(&lock);
 	if (id_priv->cma_dev) {
 		mutex_unlock(&lock);
-		switch (rdma_node_get_transport(id->device->node_type)) {
+		switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
 		case RDMA_TRANSPORT_IB:
 			if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib))
 				ib_destroy_cm_id(id_priv->cm_id.ib);

@@ -1708,6 +1755,77 @@ static int cma_resolve_iw_route(struct rdma_id_private *id_priv, int timeout_ms)
 	return 0;
 }
 
+static int cma_resolve_iboe_route(struct rdma_id_private *id_priv)
+{
+	struct rdma_route *route = &id_priv->id.route;
+	struct rdma_addr *addr = &route->addr;
+	struct cma_work *work;
+	int ret;
+	struct sockaddr_in *src_addr = (struct sockaddr_in *)&route->addr.src_addr;
+	struct sockaddr_in *dst_addr = (struct sockaddr_in *)&route->addr.dst_addr;
+	struct net_device *ndev = NULL;
+
+	if (src_addr->sin_family != dst_addr->sin_family)
+		return -EINVAL;
+
+	work = kzalloc(sizeof *work, GFP_KERNEL);
+	if (!work)
+		return -ENOMEM;
+
+	work->id = id_priv;
+	INIT_WORK(&work->work, cma_work_handler);
+
+	route->path_rec = kzalloc(sizeof *route->path_rec, GFP_KERNEL);
+	if (!route->path_rec) {
+		ret = -ENOMEM;
+		goto err1;
+	}
+
+	route->num_paths = 1;
+
+	iboe_mac_to_ll(&route->path_rec->sgid, addr->dev_addr.src_dev_addr);
+	iboe_mac_to_ll(&route->path_rec->dgid, addr->dev_addr.dst_dev_addr);
+
+	route->path_rec->hop_limit = 1;
+	route->path_rec->reversible = 1;
+	route->path_rec->pkey = cpu_to_be16(0xffff);
+	route->path_rec->mtu_selector = IB_SA_EQ;
+
+	if (addr->dev_addr.bound_dev_if)
+		ndev = dev_get_by_index(&init_net, addr->dev_addr.bound_dev_if);
+	if (!ndev) {
+		ret = -ENODEV;
+		goto err2;
+	}
+
+	route->path_rec->mtu = iboe_get_mtu(ndev->mtu);
+	route->path_rec->rate_selector = IB_SA_EQ;
+	route->path_rec->rate = iboe_get_rate(ndev);
+	dev_put(ndev);
+	route->path_rec->packet_life_time_selector = IB_SA_EQ;
+	route->path_rec->packet_life_time = IBOE_PACKET_LIFETIME;
+	if (!route->path_rec->mtu) {
+		ret = -EINVAL;
+		goto err2;
+	}
+
+	work->old_state = CMA_ROUTE_QUERY;
+	work->new_state = CMA_ROUTE_RESOLVED;
+	work->event.event = RDMA_CM_EVENT_ROUTE_RESOLVED;
+	work->event.status = 0;
+
+	queue_work(cma_wq, &work->work);
+
+	return 0;
+
+err2:
+	kfree(route->path_rec);
+	route->path_rec = NULL;
+err1:
+	kfree(work);
+	return ret;
+}
+
 int rdma_resolve_route(struct rdma_cm_id *id, int timeout_ms)
 {
 	struct rdma_id_private *id_priv;

@@ -1720,7 +1838,16 @@ int rdma_resolve_route(struct rdma_cm_id *id, int timeout_ms)
 	atomic_inc(&id_priv->refcount);
 	switch (rdma_node_get_transport(id->device->node_type)) {
 	case RDMA_TRANSPORT_IB:
-		ret = cma_resolve_ib_route(id_priv, timeout_ms);
+		switch (rdma_port_get_link_layer(id->device, id->port_num)) {
+		case IB_LINK_LAYER_INFINIBAND:
+			ret = cma_resolve_ib_route(id_priv, timeout_ms);
+			break;
+		case IB_LINK_LAYER_ETHERNET:
+			ret = cma_resolve_iboe_route(id_priv);
+			break;
+		default:
+			ret = -ENOSYS;
+		}
 		break;
 	case RDMA_TRANSPORT_IWARP:
 		ret = cma_resolve_iw_route(id_priv, timeout_ms);

@@ -1773,7 +1900,7 @@ port_found:
 		goto out;
 
 	id_priv->id.route.addr.dev_addr.dev_type =
-		(rdma_node_get_transport(cma_dev->device->node_type) == RDMA_TRANSPORT_IB) ?
+		(rdma_port_get_link_layer(cma_dev->device, p) == IB_LINK_LAYER_INFINIBAND) ?
 		ARPHRD_INFINIBAND : ARPHRD_ETHER;
 
 	rdma_addr_set_sgid(&id_priv->id.route.addr.dev_addr, &gid);

@@ -2758,6 +2885,103 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
 	return 0;
 }
 
+
+static void iboe_mcast_work_handler(struct work_struct *work)
+{
+	struct iboe_mcast_work *mw = container_of(work, struct iboe_mcast_work, work);
+	struct cma_multicast *mc = mw->mc;
+	struct ib_sa_multicast *m = mc->multicast.ib;
+
+	mc->multicast.ib->context = mc;
+	cma_ib_mc_handler(0, m);
+	kref_put(&mc->mcref, release_mc);
+	kfree(mw);
+}
+
+static void cma_iboe_set_mgid(struct sockaddr *addr, union ib_gid *mgid)
+{
+	struct sockaddr_in *sin = (struct sockaddr_in *)addr;
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
+
+	if (cma_any_addr(addr)) {
+		memset(mgid, 0, sizeof *mgid);
+	} else if (addr->sa_family == AF_INET6)
+		memcpy(mgid, &sin6->sin6_addr, sizeof *mgid);
+	else {
+		mgid->raw[0] = 0xff;
+		mgid->raw[1] = 0x0e;
+		mgid->raw[2] = 0;
+		mgid->raw[3] = 0;
+		mgid->raw[4] = 0;
+		mgid->raw[5] = 0;
+		mgid->raw[6] = 0;
+		mgid->raw[7] = 0;
+		mgid->raw[8] = 0;
+		mgid->raw[9] = 0;
+		mgid->raw[10] = 0xff;
+		mgid->raw[11] = 0xff;
+		*(__be32 *)(&mgid->raw[12]) = sin->sin_addr.s_addr;
+	}
+}
+
+static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
+				   struct cma_multicast *mc)
+{
+	struct iboe_mcast_work *work;
+	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
+	int err;
+	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
+	struct net_device *ndev = NULL;
+
+	if (cma_zero_addr((struct sockaddr *)&mc->addr))
+		return -EINVAL;
+
+	work = kzalloc(sizeof *work, GFP_KERNEL);
+	if (!work)
+		return -ENOMEM;
+
+	mc->multicast.ib = kzalloc(sizeof(struct ib_sa_multicast), GFP_KERNEL);
+	if (!mc->multicast.ib) {
+		err = -ENOMEM;
+		goto out1;
+	}
+
+	cma_iboe_set_mgid(addr, &mc->multicast.ib->rec.mgid);
+
+	mc->multicast.ib->rec.pkey = cpu_to_be16(0xffff);
+	if (id_priv->id.ps == RDMA_PS_UDP)
+		mc->multicast.ib->rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
+
+	if (dev_addr->bound_dev_if)
+		ndev = dev_get_by_index(&init_net, dev_addr->bound_dev_if);
+	if (!ndev) {
+		err = -ENODEV;
+		goto out2;
+	}
+	mc->multicast.ib->rec.rate = iboe_get_rate(ndev);
+	mc->multicast.ib->rec.hop_limit = 1;
+	mc->multicast.ib->rec.mtu = iboe_get_mtu(ndev->mtu);
+	dev_put(ndev);
+	if (!mc->multicast.ib->rec.mtu) {
+		err = -EINVAL;
+		goto out2;
+	}
+	iboe_addr_get_sgid(dev_addr, &mc->multicast.ib->rec.port_gid);
+	work->id = id_priv;
+	work->mc = mc;
+	INIT_WORK(&work->work, iboe_mcast_work_handler);
+	kref_get(&mc->mcref);
+	queue_work(cma_wq, &work->work);
+
+	return 0;
+
+out2:
+	kfree(mc->multicast.ib);
+out1:
+	kfree(work);
+	return err;
+}
+
 int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
 			void *context)
 {

@@ -2784,7 +3008,17 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
 
 	switch (rdma_node_get_transport(id->device->node_type)) {
 	case RDMA_TRANSPORT_IB:
-		ret = cma_join_ib_multicast(id_priv, mc);
+		switch (rdma_port_get_link_layer(id->device, id->port_num)) {
+		case IB_LINK_LAYER_INFINIBAND:
+			ret = cma_join_ib_multicast(id_priv, mc);
+			break;
+		case IB_LINK_LAYER_ETHERNET:
+			kref_init(&mc->mcref);
+			ret = cma_iboe_join_multicast(id_priv, mc);
+			break;
+		default:
+			ret = -EINVAL;
+		}
 		break;
 	default:
 		ret = -ENOSYS;

@@ -2797,6 +3031,7 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
 		spin_unlock_irq(&id_priv->lock);
 		kfree(mc);
 	}
+
 	return ret;
 }
 EXPORT_SYMBOL(rdma_join_multicast);

@@ -2817,8 +3052,19 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr)
 				ib_detach_mcast(id->qp,
 						&mc->multicast.ib->rec.mgid,
 						mc->multicast.ib->rec.mlid);
-			ib_sa_free_multicast(mc->multicast.ib);
-			kfree(mc);
+			if (rdma_node_get_transport(id_priv->cma_dev->device->node_type) == RDMA_TRANSPORT_IB) {
+				switch (rdma_port_get_link_layer(id->device, id->port_num)) {
+				case IB_LINK_LAYER_INFINIBAND:
+					ib_sa_free_multicast(mc->multicast.ib);
+					kfree(mc);
+					break;
+				case IB_LINK_LAYER_ETHERNET:
+					kref_put(&mc->mcref, release_mc);
+					break;
+				default:
+					break;
+				}
+			}
 			return;
 		}
 	}
drivers/infiniband/core/sa_query.c +4 -1
index 27674c7..91a6603 100644
@@ -496,6 +496,7 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num,
 {
 	int ret;
 	u16 gid_index;
+	int force_grh;
 
 	memset(ah_attr, 0, sizeof *ah_attr);
 	ah_attr->dlid = be16_to_cpu(rec->dlid);

@@ -505,7 +506,9 @@ int ib_init_ah_from_path(struct ib_device *device, u8 port_num,
 	ah_attr->port_num = port_num;
 	ah_attr->static_rate = rec->rate;
 
-	if (rec->hop_limit > 1) {
+	force_grh = rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_ETHERNET;
+
+	if (rec->hop_limit > 1 || force_grh) {
 		ah_attr->ah_flags = IB_AH_GRH;
 		ah_attr->grh.dgid = rec->dgid;
drivers/infiniband/core/ucma.c +39 -6

57 lines not on this page · raw message

include/rdma/ib_addr.h +97 -1

122 lines not on this page · raw message

Signature · 6 lines
1.7.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#1 lore raw

drivers/infiniband/core/cma.c static int cma_acquire_dev(struct rdma_id_private *id_priv) · line 352

Quoted 7 lines
drivers/infiniband/core/cma.c +2 -1
@@ -330,15 +348,29 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv)
 	union ib_gid gid;
 	int ret = -ENODEV;
 
-	rdma_addr_get_sgid(dev_addr, &gid);
+	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
+		iboe_addr_get_sgid(dev_addr, &gid);

How does this work with iwarp?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CF9C39F99A89134C9CF9C4CCB68B8DDF25AB26491D-osO9UTpF0USkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org

author
#3 lore raw
Quoted 10 lines · Sean

drivers/infiniband/core/cma.c static int cma_acquire_dev(struct rdma_id_private *id_priv) · line 352

Quoted 7 lines
drivers/infiniband/core/cma.c +2 -1
@@ -330,15 +348,29 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv)
 	union ib_gid gid;
 	int ret = -ENODEV;
 
-	rdma_addr_get_sgid(dev_addr, &gid);
+	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
+		iboe_addr_get_sgid(dev_addr, &gid);

How does this work with iwarp?

Referring to the entire function:

static int cma_acquire_dev(struct rdma_id_private *id_priv)

{
	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
	struct cma_device *cma_dev;
	union ib_gid gid;
	int ret = -ENODEV;

	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
		iboe_addr_get_sgid(dev_addr, &gid);
		list_for_each_entry(cma_dev, &dev_list, list) {
			ret = ib_find_cached_gid(cma_dev->device, &gid,
						 &id_priv->id.port_num, NULL);
			if (!ret)
				goto out;
		}
	}

	memcpy(&gid, dev_addr->src_dev_addr +
	       rdma_addr_gid_offset(dev_addr), sizeof gid);
	list_for_each_entry(cma_dev, &dev_list, list) {
		ret = ib_find_cached_gid(cma_dev->device, &gid,
					 &id_priv->id.port_num, NULL);
		if (!ret)
			break;
	}

out:

	if (!ret)
		cma_attach_to_dev(id_priv, cma_dev);

	return ret;
}

The idea is that if it is not infiniband then it is either iwarp or iboe. So we first build a gid assuming it is the iboe case and search it the devices' gid tables. If it was not found then we proceed with the regular flow.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 19 lines
	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
		iboe_addr_get_sgid(dev_addr, &gid);
		list_for_each_entry(cma_dev, &dev_list, list) {
			ret = ib_find_cached_gid(cma_dev->device, &gid,
						 &id_priv->id.port_num, NULL);
			if (!ret)
				goto out;
		}
	}

	memcpy(&gid, dev_addr->src_dev_addr +
	       rdma_addr_gid_offset(dev_addr), sizeof gid);
	list_for_each_entry(cma_dev, &dev_list, list) {
		ret = ib_find_cached_gid(cma_dev->device, &gid,
					 &id_priv->id.port_num, NULL);
		if (!ret)
			break;
	}
Quoted 1 line

out:

This duplication of the code in the loop ends up looking pretty bad, and indeed the change to the function doesn't make much sense to me:

Quoted 7 lines
drivers/infiniband/core/cma.c +2 -1
@@ -330,15 +348,29 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv)
 	union ib_gid gid;
 	int ret = -ENODEV;
 
-	rdma_addr_get_sgid(dev_addr, &gid);
+	if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
+		iboe_addr_get_sgid(dev_addr, &gid);

drivers/infiniband/core/cma.c static int cma_acquire_dev(struct rdma_id_private *id_priv) · line 362

Quoted 10 lines
drivers/infiniband/core/cma.c +10 -0
+		list_for_each_entry(cma_dev, &dev_list, list) {
+			ret = ib_find_cached_gid(cma_dev->device, &gid,
+						 &id_priv->id.port_num, NULL);
+			if (!ret)
+				goto out;
+		}
+	}
+
+	memcpy(&gid, dev_addr->src_dev_addr +
+	       rdma_addr_gid_offset(dev_addr), sizeof gid);

I thought the whole point of this change:

include/rdma/ib_addr.h static inline int rdma_addr_gid_offset(struct rdma_dev_addr *dev_addr) · line 157

Quoted 10 lines
include/rdma/ib_addr.h +6 -1
 static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid)
 {
-	memcpy(gid, dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr), sizeof *gid);
+	if (dev_addr->transport == RDMA_TRANSPORT_IB &&
+	    dev_addr->dev_type != ARPHRD_INFINIBAND)
+		iboe_addr_get_sgid(dev_addr, gid);
+	else
+		memcpy(gid, dev_addr->src_dev_addr +
+		       rdma_addr_gid_offset(dev_addr), sizeof *gid);
 }

is that rdma_addr_get_sgid() now calls iboe_addr_get_sgid for IBoE devices, and does the original thing in the other case. So I don't see why any change to cma_acquire_dev() is needed at all?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw
Quoted 18 lines · Roland Dreier

I thought the whole point of this change:

include/rdma/ib_addr.h static inline int rdma_addr_gid_offset(struct rdma_dev_addr *dev_addr) · line 157

Quoted 10 lines

static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid) {

include/rdma/ib_addr.h +6 -1
-	memcpy(gid, dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr), sizeof *gid);
+	if (dev_addr->transport == RDMA_TRANSPORT_IB &&
+	    dev_addr->dev_type != ARPHRD_INFINIBAND)
+		iboe_addr_get_sgid(dev_addr, gid);
+	else
+		memcpy(gid, dev_addr->src_dev_addr +
+		       rdma_addr_gid_offset(dev_addr), sizeof *gid);
 }

is that rdma_addr_get_sgid() now calls iboe_addr_get_sgid for IBoE devices, and does the original thing in the other case. So I don't see why any change to cma_acquire_dev() is needed at all?

The point is it cma_acquire_dev() may get called when dev_addr->transport has not yet been set so we can't rely on its value to retrieve the correct GID; for example when it gets called by rdma_bind_addr() it will only set the transport a few lines down the function when calling cma_attach_to_dev(). That's why I may have to look for the GID in the IBoE case.
I agree that traversing the list twice does not look so good so how about this:

static int cma_acquire_dev(struct rdma_id_private *id_priv)

{
	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
	struct cma_device *cma_dev;
	union ib_gid gid, iboe_gid;
	int ret = -ENODEV;

	iboe_addr_get_sgid(dev_addr, &iboe_gid);
	memcpy(&gid, dev_addr->src_dev_addr +
	       rdma_addr_gid_offset(dev_addr), sizeof gid);
	list_for_each_entry(cma_dev, &dev_list, list) {
		ret = ib_find_cached_gid(cma_dev->device, &gid,
					 &id_priv->id.port_num, NULL);
		if (!ret)
			break;
		if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
			ret = ib_find_cached_gid(cma_dev->device, &iboe_gid,
						 &id_priv->id.port_num, NULL);
			if (!ret)
				break;
		}
	}

	if (!ret)
		cma_attach_to_dev(id_priv, cma_dev);

	return ret;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 8 lines

The point is it cma_acquire_dev() may get called when dev_addr->transport has not yet been set so we can't rely on its value to retrieve the correct GID; for example when it gets called by rdma_bind_addr() it will only set the transport a few lines down the function when calling cma_attach_to_dev(). That's why I may have to look for the GID in the IBoE case.
I agree that traversing the list twice does not look so good so how about this:

I dislike the idea of guessing where the GID is when trying to perform these matches. Is there any chance of getting a false match?

Is there some other approach that anyone can think of for handling this? Maybe register the netdev HW address with ib_addr or rdma_cm? --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw
In reply to Eli Cohen
Quoted 6 lines

The point is it cma_acquire_dev() may get called when dev_addr->transport has not yet been set so we can't rely on its value to retrieve the correct GID; for example when it gets called by rdma_bind_addr() it will only set the transport a few lines down the function when calling cma_attach_to_dev(). That's why I may have to look for the GID in the IBoE case.

I see. We just have the address but the RDMA CM API doesn't give us the type exactly (we just know the ARPHRD type).

Quoted 4 lines

I agree that traversing the list twice does not look so good so how about this:

static int cma_acquire_dev(struct rdma_id_private *id_priv)

Quoted 9 lines
{
	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
	struct cma_device *cma_dev;
	union ib_gid gid, iboe_gid;
	int ret = -ENODEV;

	iboe_addr_get_sgid(dev_addr, &iboe_gid);
	memcpy(&gid, dev_addr->src_dev_addr +
	       rdma_addr_gid_offset(dev_addr), sizeof gid);

I wish there were a cleaner way to write all this...

Quoted 11 lines
	list_for_each_entry(cma_dev, &dev_list, list) {
		ret = ib_find_cached_gid(cma_dev->device, &gid,
					 &id_priv->id.port_num, NULL);
		if (!ret)
			break;
		if (dev_addr->dev_type != ARPHRD_INFINIBAND) {
			ret = ib_find_cached_gid(cma_dev->device, &iboe_gid,
						 &id_priv->id.port_num, NULL);
			if (!ret)
				break;
		}

looks better but since we know everything about the device we're comparing to, can't we do something like:

		if (addr and dev ARPHRD agree)
			if (dev is IBoE)
				ret = ib_find_gid(iboe_gid);
			else
				ret = ib_find_gid(gid);

		if (!ret)
			break;

(by the way, it seems we can use ib_find_gid() here since we are in a sleepable context -- no need to rely on the cached_gid stuff, which I would like to phase out someday)

Quoted 7 lines
	}

	if (!ret)
		cma_attach_to_dev(id_priv, cma_dev);

	return ret;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#10 lore raw
Quoted 4 lines · Roland Dreier

looks better but since we know everything about the device we're comparing to, can't we do something like:

Quoted 8 lines
		if (addr and dev ARPHRD agree)
			if (dev is IBoE)
				ret = ib_find_gid(iboe_gid);
			else
				ret = ib_find_gid(gid);

		if (!ret)
			break;

This looks even better with no duplicate checks.

Quoted 5 lines

(by the way, it seems we can use ib_find_gid() here since we are in a sleepable context -- no need to rely on the cached_gid stuff, which I would like to phase out someday)

Sure, I can change that too.
Please let me know if you'd like me to send a new series or just send this patch again.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#13 lore raw
Quoted 1 line · Roland Dreier

Please just resend this, thanks.

Here's the new code. I added a function that tries to find a match for a given port, istead of ib_find_cached_gid() which searches all the ports.
I tested this on a machine with two cards with different configurations and it works fine.

static int find_gid_port(struct ib_device *device, union ib_gid *gid, u8 port_num)

{
	int i;
	int err;
	struct ib_port_attr props;
	union ib_gid tmp;

	err = ib_query_port(device, port_num, &props);
	if (err)
		return 1;

	for (i = 0; i < props.gid_tbl_len; ++i) {
		err = ib_query_gid(device, port_num, i, &tmp);
		if (err)
			return 1;
		if (!memcmp(&tmp, gid, sizeof tmp))
			return 0;
	}

	return -EAGAIN;
}

static int cma_acquire_dev(struct rdma_id_private *id_priv)

{
	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
	struct cma_device *cma_dev;
	union ib_gid gid, iboe_gid;
	int ret = -ENODEV;
	u8 port;
	enum rdma_link_layer dev_ll = dev_addr->dev_type == ARPHRD_INFINIBAND ?
		IB_LINK_LAYER_INFINIBAND : IB_LINK_LAYER_ETHERNET;

	iboe_addr_get_sgid(dev_addr, &iboe_gid);
	memcpy(&gid, dev_addr->src_dev_addr +
	       rdma_addr_gid_offset(dev_addr), sizeof gid);
	list_for_each_entry(cma_dev, &dev_list, list) {
		for (port = 1; port <= cma_dev->device->phys_port_cnt; ++port) {
			if (rdma_port_get_link_layer(cma_dev->device, port) == dev_ll) {
				if (rdma_node_get_transport(cma_dev->device->node_type) == RDMA_TRANSPORT_IB &&
				    rdma_port_get_link_layer(cma_dev->device, port) == IB_LINK_LAYER_ETHERNET)
					ret = find_gid_port(cma_dev->device, &iboe_gid, port);
				else
					ret = find_gid_port(cma_dev->device, &gid, port);

				if (!ret) {
					id_priv->id.port_num = port;
					goto out;
				} else if (ret == 1)
					break;
			}
		}
	}

out:

	if (!ret)
		cma_attach_to_dev(id_priv, cma_dev);

	return ret;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw
In reply to Eli Cohen
Quoted 9 lines
Quoted 9 lines

looks better but since we know everything about the device we're comparing to, can't we do something like:

		if (addr and dev ARPHRD agree)
			if (dev is IBoE)
				ret = ib_find_gid(iboe_gid);
			else
				ret = ib_find_gid(gid);
Quoted 4 lines
Quoted 2 lines
		if (!ret)
			break;

This looks even better with no duplicate checks.

In cma_acquire_dev, we try to match up a portion of the dev_addr with a GID. The current code does this by checking the dev_type to determine if the GID would start at offset 0 or 4 in the dev_addr. With IBoE, this check is no longer possible, so we select the offset based on the rdma device that we're trying to compare against, rather than using data stored with the net_device.

Maybe it's just so unlikely that we don't care, but can this lead to a false match? (and maybe the current code can have a false match..) --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CF9C39F99A89134C9CF9C4CCB68B8DDF25AB44BA6F-osO9UTpF0USkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org

#16 lore raw
Quoted 9 lines

In cma_acquire_dev, we try to match up a portion of the dev_addr with a GID. The current code does this by checking the dev_type to determine if the GID would start at offset 0 or 4 in the dev_addr. With IBoE, this check is no longer possible, so we select the offset based on the rdma device that we're trying to compare against, rather than using data stored with the net_device.

Maybe it's just so unlikely that we don't care, but can this lead to a false match? (and maybe the current code can have a false match..)

I don't think there's any chance of a false positive. We can tell if the GID offset is 0 or 4 based on the ARPHRD type of the address we're comparing, so that should always be correct. The only difference with between IBoE and iWARP GIDs is that the IBoE GIDs are actually IPv6 link-local addresses, so they need some munging to convert back to ethernet hardware addresses. But in any case there shouldn't be a collision unless there are duplicate ethernet addresses.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html