[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: [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.

-- 
MST