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

[Yosef Etigin]

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.

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.