[ofa-general] Re: [PATCHv2 1/2] core: uncached "find gid" and "find pkey" queries

[Michael S. Tsirkin]

> Quoting Yosef Etigin <yosefe at voltaire.com>:
> Subject: Re: [PATCHv2 1/2] core: uncached "find gid" and "find pkey" queries
> 
> Michael S. Tsirkin wrote:
> >>Quoting Yosef Etigin <yosefe at voltaire.com>:
> >>Subject: [PATCHv2 1/2] core: uncached "find gid" and "find pkey" queries
> >>
> >>Add ib_find_gid and ib_find_pkey over uncached device queries.
> >>The calls might block but the returns are always up-to-date.
> >>
> >>
> >>Signed-off-by: Yosef Etigin <yosefe at voltaire.com>
> >>---
> >> drivers/infiniband/core/device.c |   96 +++++++++++++++++++++++++++++++++++++++
> >> include/rdma/ib_verbs.h          |   23 +++++++++
> >> 2 files changed, 119 insertions(+)
> >>
> >>Index: b/drivers/infiniband/core/device.c
> >>===================================================================
> >>--- a/drivers/infiniband/core/device.c	2007-05-07 15:42:19.000000000 +0300
> >>+++ b/drivers/infiniband/core/device.c	2007-05-08 11:16:35.049600754 +0300
> >>@@ -149,6 +149,18 @@ static int alloc_name(char *name)
> >> 	return 0;
> >> }
> >> 
> >>+static inline int start_port(struct ib_device *device)
> >>+{
> >>+	return (device->node_type == RDMA_NODE_IB_SWITCH) ? 0 : 1;
> >>+}
> >>+
> >>+
> >>+static inline int end_port(struct ib_device *device)
> >>+{
> >>+	return (device->node_type == RDMA_NODE_IB_SWITCH) ?
> >>+		0 : device->phys_port_cnt;
> >>+}
> >>+
> >> /**
> >>  * ib_alloc_device - allocate an IB device struct
> >>  * @size:size of structure to allocate
> >>@@ -592,6 +604,90 @@ int ib_modify_port(struct ib_device *dev
> >> }
> >> EXPORT_SYMBOL(ib_modify_port);
> >> 
> >>+/**
> >>+ * ib_find_gid - Returns the port number and GID table index where
> >>+ *   a specified GID value occurs.
> >>+ * @device: The device to query.
> >>+ * @gid: The GID value to search for.
> >>+ * @port_num: The port number of the device where the GID value was found.
> >>+ * @index: The index into the GID table where the GID was found.  This
> >>+ *   parameter may be NULL.
> >>+ */
> >>+int ib_find_gid(struct ib_device *device, union ib_gid *gid,
> >>+		       u8 *port_num, u16 *index)
> >>+{
> >>+	struct ib_port_attr *tprops = NULL;
> >>+	union ib_gid tmp_gid;
> >>+	int ret, port, i;
> >>+
> >>+	tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
> >>+
> >>+	for (port = start_port(device); port <= end_port(device); ++port) {
> >>+		ret = ib_query_port(device, port, tprops);
> >>+		if (ret)
> >>+			continue;
> >>+
> >>+		for (i = 0; i < tprops->gid_tbl_len; ++i) {
> >>+			ret = ib_query_gid(device, port, i, &tmp_gid);
> >>+			if (ret)
> >>+				goto out;
> >>+			if (!memcmp(&tmp_gid, gid, sizeof *gid)) {
> >>+				*port_num = port;
> >>+				*index = i;
> >>+				ret = 0;
> >>+				goto out;
> >>+			}
> >>+		}
> >>+	}
> >>+	ret = -ENOENT;
> >>+out:
> >>+	kfree(tprops);
> >>+	return ret;
> >>+}
> >>+EXPORT_SYMBOL(ib_find_gid);
> >>+
> >>+/**
> >>+ * ib_find_pkey - Returns the PKey table index where a specified
> >>+ *   PKey value occurs.
> >>+ * @device: The device to query.
> >>+ * @port_num: The port number of the device to search for the PKey.
> >>+ * @pkey: The PKey value to search for.
> >>+ * @index: The index into the PKey table where the PKey was found.
> >>+ */
> >>+int ib_find_pkey(struct ib_device *device,
> >>+			u8 port_num, u16 pkey, u16 *index)
> >>+{
> >>+	struct ib_port_attr *tprops = NULL;
> >>+	int ret, i;
> >>+	u16 tmp_pkey;
> >>+
> >>+	tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
> >>+
> >>+	ret = ib_query_port(device, port_num, tprops);
> >>+	if (ret) {
> >>+		printk(KERN_WARNING "ib_query_port failed , ret = %d\n", ret);
> >>+		goto out;
> >>+	}
> >>+
> >>+	for (i = 0; i < tprops->pkey_tbl_len; ++i) {
> >>+		ret = ib_query_pkey(device, port_num, i, &tmp_pkey);
> >>+		if (ret)
> >>+			goto out;
> >>+
> >>+		if (pkey == tmp_pkey) {
> >>+			*index = i;
> >>+			ret = 0;
> >>+			goto out;
> >>+		}
> >>+	}
> >>+	ret = -ENOENT;
> >>+
> >>+out:
> >>+	kfree(tprops);
> >>+	return ret;
> >>+}
> >>+EXPORT_SYMBOL(ib_find_pkey);
> >>+
> >> static int __init ib_core_init(void)
> >> {
> >> 	int ret;
> > 
> > 
> > OK, look good - later, providers will be able to optimize these
> > by caching ib_query_pkey/ib_query_gid calls.
> > 
> > But I see a problem here in that ib_query_port is a call providers
> > won't be able to optimize out (because it includes e.g. port state),
> > and it seems a waste.
> > 
> > Is that right?
> > 
> > One way out would be to pass the table length in to ib_find_pkey/ib_find_gid
> > as an extra parameter, and cache that at the ULP level.
> > 
> provider might try to remember the port state after each mad we see.. but it
> looks like too much to demand from it.

Port can go down without any MADs.

> Anyway, since the information about the port table length does not come from mads
> but from device properties, the core can set each of these lengths during initiallization,
> and use them in ib_find_* functions.

Passing it in looks simpler ... but maybe you're right.
Patch?

-- 
MST