Re: [PATCH V2 5/5] RDMA CM: Netlink Client

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Mon, Nov 29, 2010 at 06:16:39PM +0200, Nir Muchtar wrote:
> Add callbacks and data types for statistics export.
> One callback is implemented that exports all of the current devices/ids.
> Add/remove the callback to IB Netlink on init/cleanup.

Please include the schema for the messages you are adding to netlink
in the comment so other people can review them easily.

Looks to me like you have messages of the form:

{
 [IBNL_RDMA_CM_DEVICE_NAME - char[]]
 [IBNL_RDMA_CM_ID_STATS - struct rdma_cm_id_stats]*
}*

As I've said before, I don't want to see this tied to RDMA_CM, that is
not general enough for a new userspace API. The use of
IBNL_RDMA_CM_DEVICE_NAME is very un-netlink-like and is just an ugly
hack to avoid addressing that problem.

How about messages of the form:
{
 IBNL_QP - struct ib_nl_qp
   IBNL_QP_ATTR - struct ib_qp_attr (or a reasonable subset)
   IBNL_QP_CM_INFO u8[] - struct ib_nl_qp_cm_info
   IBNL_QP_CM_SERVICE_ID u8[]
   IBNL_RDMA_CM_INFO - struct ib_nl_rdma_cm_info
   IBNL_RDMA_CM_SRC - u8[]  // This is a sockaddr_*
   IBNL_RDMA_CM_DST - u8[] 
}+

Meaning there is an array of IBNL_QP messages which contains various
attributes. Similar to how everything else in netlink works.

struct ib_nl_qp
{
        // String names for IB devices was a mistake, don't perpetuate it.
        __u32 ib_dev_if;
	__u32 qp_num;
        __s32 creator_pid; // -1 for kernel consumer
};

struct ib_nl_qp_cm_info
{
      u32 cm_state; // enum ib_cm_state
      u32 lap_state;
};

struct ib_nl_rdma_cm_info
{
	__u32 bound_dev_if;
        __u32 family;
	__u32 cm_state; // enum rdma_cm_state
};

This captures more information and doesn't tie things to RDMA_CM.

iWarp QPs would not export IBNL_QP_CM_INFO and QP_CM_SERVICE_ID, but
ideally we'd have a call out to the NIC to include the TCP diag
information for the underlying TCP socket since there is no other way
to access that.

Non RDMA-CM QPs (ie ipoib) would not include the RDMA_CM bits.

If you study how SS works you'll see it is similar, it uses a message
of type AF_INET/6.. and then includes attributes like
INET_DIAG_MEMINFO/INFO/CONG

> @@ -134,6 +135,7 @@ struct rdma_id_private {
>  	u32			qp_num;
>  	u8			srq;
>  	u8			tos;
> +	pid_t			owner;

Maybe a seperate patch for this? It probably really belongs in
ib_uverbs. What about kernel consumers?

> +static int cma_get_stats(struct sk_buff **nl_skb, int pid)

You really have to use netlink_dump_start here, doing it like this
will deadlock. See how other places use NLM_F_DUMP as well.

> +struct rdma_cm_id_stats {
> +	enum rdma_node_type nt;
> +	int port_num;
> +	int bound_dev_if;
> +	__be16	local_port;
> +	__be16	remote_port;
> +	__be32	local_addr[4];
> +	__be32	remote_addr[4];
> +	enum rdma_port_space ps;
> +	enum rdma_cm_state cm_state;
> +	u32 qp_num;
> +	pid_t pid;
> +};

Putting enums in a user/kernel structure like this makes me nervous
that we'll have a 32/64 bit problem. It would be more consistent with
the uverbs stuff to use explicit fixed width types.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html