RE: [PATCH RFC iproute-next 2/5] rdma: Add CM_ID resource tracking information

["Steve Wise" <swise@opengridcomputing.com>]

> 
> On Wed, Feb 14, 2018 at 01:07:01PM -0800, Steve Wise wrote:
> > Sample output:
> >
> > # rdma resource
> > 2: cxgb4_0: pd 5 cq 2 qp 2 cm_id 3 mr 7
> > 3: mlx4_0: pd 7 cq 3 qp 3 cm_id 3 mr 7
> >
> > # rdma resource show cm_id
> > [root@stevo1 iproute2]# /root/stevo/iproute2/rdma/rdma resource  show
> cm_id
> > link cxgb4_0/- lqpn 0 qp-type RC state LISTEN ps TCP dev-type ---
transport-
> type IWARP pid 30485 comm rping src-addr 0.0.0.0 src-port 7174 dst-addr
> 0.0.0.0 dst-port 0
> > link cxgb4_0/2 lqpn 1048 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IWARP pid 30503 comm rping src-addr 172.16.2.1 src-port
> 7174 dst-addr 172.16.2.1 dst-port 38246
> > link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IWARP pid 30498 comm rping src-addr 172.16.2.1 src-port
> 38246 dst-addr 172.16.2.1 dst-port 7174
> > link mlx4_0/- lqpn 0 qp-type RC state LISTEN ps TCP dev-type ---
transport-
> type IB pid 30485 comm rping src-addr 0.0.0.0 src-port 7174 dst-addr
0.0.0.0
> dst-port 0
> > link mlx4_0/1 lqpn 539 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IB pid 30494 comm rping src-addr 172.16.99.1 src-port 7174
> dst-addr 172.16.99.1 dst-port 43670
> > link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IB pid 30492 comm rping src-addr 172.16.99.1 src-port 43670
> dst-addr 172.16.99.1 dst-port 7174
> >
> > # rdma resource show cm_id dst-port 7174
> > link cxgb4_0/2 lqpn 1040 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IWARP pid 30498 comm rping src-addr 172.16.2.1 src-port
> 38246 dst-addr 172.16.2.1 dst-port 7174
> > link mlx4_0/1 lqpn 538 qp-type RC state CONNECT ps TCP dev-type ETH
> transport-type IB pid 30492 comm rping src-addr 172.16.99.1 src-port 43670
> dst-addr 172.16.99.1 dst-port 7174
> >
> > Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > ---
> >  rdma/rdma.h  |   1 +
> >  rdma/res.c   | 312
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  rdma/utils.c |  12 +++
> >  3 files changed, 321 insertions(+), 4 deletions(-)
> 
> Thanks, for doing it.
> 
> >
> > diff --git a/rdma/rdma.h b/rdma/rdma.h
> > index 5809f70..1ef0942 100644
> > --- a/rdma/rdma.h
> > +++ b/rdma/rdma.h
> > @@ -18,6 +18,7 @@
> >  #include <libmnl/libmnl.h>
> >  #include <rdma/rdma_netlink.h>
> >  #include <time.h>
> > +#include <net/if_arp.h>
> >
> >  #include "list.h"
> >  #include "utils.h"
> > diff --git a/rdma/res.c b/rdma/res.c
> > index 2a63e71..beae7dc 100644
> > --- a/rdma/res.c
> > +++ b/rdma/res.c
> > @@ -16,9 +16,11 @@ static int res_help(struct rd *rd)
> >  {
> >  	pr_out("Usage: %s resource\n", rd->filename);
> >  	pr_out("          resource show [DEV]\n");
> > -	pr_out("          resource show [qp]\n");
> > +	pr_out("          resource show [qp|cm_id]\n");
> >  	pr_out("          resource show qp link [DEV/PORT]\n");
> >  	pr_out("          resource show qp link [DEV/PORT] [FILTER-NAME
> FILTER-VALUE]\n");
> > +	pr_out("          resource show cm_id link [DEV/PORT]\n");
> > +	pr_out("          resource show cm_id link [DEV/PORT] [FILTER-NAME
> FILTER-VALUE]\n");
> >  	return 0;
> >  }
> >
> > @@ -431,6 +433,278 @@ static int res_qp_parse_cb(const struct nlmsghdr
> *nlh, void *data)
> >  	return MNL_CB_OK;
> >  }
> >
> > +static void print_qp_type(struct rd *rd, uint32_t val)
> > +{
> > +	if (rd->json_output)
> > +		jsonw_string_field(rd->jw, "qp-type",
> > +				   qp_types_to_str(val));
> > +	else
> > +		pr_out("qp-type %s ", qp_types_to_str(val));
> > +}
> > +
> > +static const char *cm_id_state_to_str(uint8_t idx)
> > +{
> > +	static const char * const cm_id_states_str[] = { "IDLE",
> "ADDR_QUERY",
> > +						      "ADDR_RESOLVED",
> "ROUTE_QUERY", "ROUTE_RESOLVED",
> > +						      "CONNECT",
> "DISCONNECT",
> > +						      "ADDR_BOUND",
> "LISTEN", "DEVICE_REMOVAL", "DESTROYING" };
> > +
> > +	if (idx < ARRAY_SIZE(cm_id_states_str))
> > +		return cm_id_states_str[idx];
> > +	return "UNKNOWN";
> > +}
> > +
> > +enum rdma_port_space {
> > +	RDMA_PS_SDP   = 0x0001,
> 
> Do we still support this PS? It is not set in the kernel and Parav
> posted internal patch to remove it.
> 
> > +	RDMA_PS_IPOIB = 0x0002,
> > +	RDMA_PS_IB    = 0x013F,
> > +	RDMA_PS_TCP   = 0x0106,
> > +	RDMA_PS_UDP   = 0x0111,
> > +};
> > +
> > +static const char *cm_id_ps_to_str(uint32_t ps)
> > +{
> > +	switch (ps) {
> > +	case RDMA_PS_SDP:
> > +		return "SDP";
> 
> The same question


You're right.  No more PS_SDP.  Also, rdma tool should get the
rdma_port_space enum from <include/rdma/rdma_cma.h>.

> 
> > +	case RDMA_PS_IPOIB:
> > +		return "IPoIB";
> > +	case RDMA_PS_IB:
> > +		return "IPoIB";
> > +	case RDMA_PS_TCP:
> > +		return "TCP";
> > +	case RDMA_PS_UDP:
> > +		return "UDP";
> > +	default:
> > +		return "---";
> > +	}
> > +}
> > +
> > +static const char *cm_id_dev_type_to_str(uint8_t dev_type)
> > +{
> > +	switch (dev_type) {
> > +	case ARPHRD_INFINIBAND:
> > +		return "IB";
> > +	case ARPHRD_ETHER:
> > +		return "ETH";
> > +	default:
> > +		return "---";
> > +	}
> > +}
> > +
> > +static const char *cm_id_transport_type_to_str(uint8_t transport_type)
> > +{
> > +	static const char * const transport_type_str[] = { "IB", "IWARP",
> "USNIC", "USNIC/UDP" };
> > +
> 
> I know that it is part of CM_ID, but wonder if node_type of device is
> not enough. The same question goes for device type, isn't it part of
> "rdma dev .." output?
> 

I've gotten rid off dev_type, transport_type, and network_type based on
feedback from the kernel patches.  So I'll remove it from this series as
well.

> > +	if (transport_type < ARRAY_SIZE(transport_type_str))
> > +		return transport_type_str[transport_type];
> > +	return "---";
> > +}
> > +
> > +static void print_cm_id_state(struct rd *rd, uint8_t state)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_string_field(rd->jw, "state",
> cm_id_state_to_str(state));
> > +		return;
> > +	}
> > +	pr_out("state %s ", cm_id_state_to_str(state));
> > +}
> > +
> > +static void print_ps(struct rd *rd, uint32_t ps)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_string_field(rd->jw, "ps", cm_id_ps_to_str(ps));
> > +		return;
> > +	}
> > +	pr_out("ps %s ", cm_id_ps_to_str(ps));
> > +}
> > +
> > +static void print_dev_type(struct rd *rd, uint8_t dev_type)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_string_field(rd->jw, "dev-type",
> cm_id_dev_type_to_str(dev_type));
> > +		return;
> > +	}
> > +	pr_out("dev-type %s ", cm_id_dev_type_to_str(dev_type));
> > +}
> > +
> > +static void print_transport_type(struct rd *rd, uint8_t transport_type)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_string_field(rd->jw, "transport-type",
> cm_id_transport_type_to_str(transport_type));
> > +		return;
> > +	}
> > +	pr_out("transport-type %s ",
> cm_id_transport_type_to_str(transport_type));
> > +}
> > +
> > +static void print_ipaddr(struct rd *rd, const char *key, char *addrstr)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_string_field(rd->jw, key, addrstr);
> > +		return;
> > +	}
> > +	pr_out("%s %s ", key, addrstr);
> > +}
> > +
> > +static void print_ipport(struct rd *rd, const char *key, uint16_t
ipport)
> > +{
> > +	if (rd->json_output) {
> > +		jsonw_uint_field(rd->jw, key, ipport);
> > +		return;
> > +	}
> > +	pr_out("%s %u ", key, ipport);
> > +}
> > +
> > +static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data)
> > +{
> > +	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX] = {};
> > +	struct nlattr *nla_table, *nla_entry;
> > +	struct rd *rd = data;
> > +	const char *name;
> > +	int idx;
> > +
> > +	mnl_attr_parse(nlh, 0, rd_attr_cb, tb);
> > +	if (!tb[RDMA_NLDEV_ATTR_DEV_INDEX] ||
> > +	    !tb[RDMA_NLDEV_ATTR_DEV_NAME] ||
> > +	    !tb[RDMA_NLDEV_ATTR_RES_CM_ID])
> > +		return MNL_CB_ERROR;
> > +
> > +	name = mnl_attr_get_str(tb[RDMA_NLDEV_ATTR_DEV_NAME]);
> > +	idx =  mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
> > +	nla_table = tb[RDMA_NLDEV_ATTR_RES_CM_ID];
> > +	mnl_attr_for_each_nested(nla_entry, nla_table) {
> > +		struct nlattr *nla_line[RDMA_NLDEV_ATTR_MAX] = {};
> > +		uint8_t dev_type, transport_type;
> > +		char src_addr_str[INET6_ADDRSTRLEN];
> > +		char dst_addr_str[INET6_ADDRSTRLEN];
> > +		uint8_t *src_addr, *dst_addr;
> > +		uint16_t src_port, dst_port;
> > +		uint32_t port = 0, pid = 0;
> > +		uint8_t type, state;
> > +		uint32_t lqpn = 0, ps;
> > +		char *comm = NULL;
> > +		int err;
> > +
> > +		err = mnl_attr_parse_nested(nla_entry, rd_attr_cb,
nla_line);
> > +		if (err != MNL_CB_OK)
> > +			return -EINVAL;
> > +
> > +		if (!nla_line[RDMA_NLDEV_ATTR_RES_TYPE] ||
> > +		    !nla_line[RDMA_NLDEV_ATTR_RES_STATE] ||
> > +		    (!nla_line[RDMA_NLDEV_ATTR_RES_IPV4_SADDR] &&
> > +		     !nla_line[RDMA_NLDEV_ATTR_RES_IPV6_SADDR]) ||
> > +		    (!nla_line[RDMA_NLDEV_ATTR_RES_IPV4_DADDR] &&
> > +		     !nla_line[RDMA_NLDEV_ATTR_RES_IPV6_DADDR]) ||
> > +		    !nla_line[RDMA_NLDEV_ATTR_RES_IP_SPORT] ||
> > +		    !nla_line[RDMA_NLDEV_ATTR_RES_IP_DPORT] ||
> > +		    (!nla_line[RDMA_NLDEV_ATTR_RES_PID] &&
> > +		     !nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME])) {
> > +			return MNL_CB_ERROR;
> 
> It is unreadable, any chances to use intermediate variables with
> descriptive names?

This gets cleaned up because the kernel driver will now pass only the
sockaddr_storage up for the addresses.  It simplifies this code, so I think
it'll be cleaner on the next version (I'll send it out today or tomorrow
hopefully).

> 
> > +		}
> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_PORT_INDEX])
> > +			port =
> mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_PORT_INDEX]);
> > +
> > +		if (port && port != rd->port_idx)
> > +			continue;
> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_RES_LQPN])
> > +			lqpn =
> mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_LQPN]);
> > +		if (rd_check_is_filtered(rd, "lqpn", lqpn))
> > +			continue;
> > +
> > +		ps =
> mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PS]);
> > +		if (rd_check_is_string_filtered(rd, "ps",
cm_id_ps_to_str(ps)))
> > +			continue;
> > +
> > +		type =
> mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_TYPE]);
> > +		if (rd_check_is_string_filtered(rd, "qp-type",
> qp_types_to_str(type)))
> > +			continue;
> > +
> > +		state =
> mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_STATE]);
> > +		if (rd_check_is_string_filtered(rd, "state",
> cm_id_state_to_str(state)))
> > +			continue;
> > +
> > +		dev_type =
> mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_DEV_TYPE]);
> > +		if (rd_check_is_string_filtered(rd, "dev-type",
> cm_id_dev_type_to_str(dev_type)))
> > +			continue;
> > +
> > +		transport_type =
> mnl_attr_get_u8(nla_line[RDMA_NLDEV_ATTR_RES_TRANSPORT_TYPE]);
> > +		if (rd_check_is_string_filtered(rd, "transport-type",
> cm_id_transport_type_to_str(transport_type)))
> > +			continue;
> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) {
> > +			pid =
> mnl_attr_get_u32(nla_line[RDMA_NLDEV_ATTR_RES_PID]);
> > +			comm = get_task_name(pid);
> > +		}
> > +		if (rd_check_is_filtered(rd, "pid", pid))
> 
> free(comm) here
> 

Oops!  Thanks!

> > +			continue;
> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]) {
> > +			/* discard const from mnl_attr_get_str */
> > +			comm = (char
> *)mnl_attr_get_str(nla_line[RDMA_NLDEV_ATTR_RES_KERN_NAME]);
> > +		}
> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_RES_IPV4_SADDR]) {
> > +			if (!nla_line[RDMA_NLDEV_ATTR_RES_IPV4_DADDR])
> > +				return -EINVAL;
> > +			src_addr =
> mnl_attr_get_payload(nla_line[RDMA_NLDEV_ATTR_RES_IPV4_SADDR]);
> > +			if (!inet_ntop(AF_INET, src_addr, src_addr_str,
> INET6_ADDRSTRLEN))
> > +				return -EINVAL;
> > +			dst_addr =
> mnl_attr_get_payload(nla_line[RDMA_NLDEV_ATTR_RES_IPV4_DADDR]);
> > +			if (!inet_ntop(AF_INET, dst_addr, dst_addr_str,
> INET6_ADDRSTRLEN))
> > +				return -EINVAL;
> > +		} else {
> > +			if (!nla_line[RDMA_NLDEV_ATTR_RES_IPV6_SADDR]
> ||
> > +			    !nla_line[RDMA_NLDEV_ATTR_RES_IPV6_DADDR])
> > +				return -EINVAL;
> > +			src_addr =
> mnl_attr_get_payload(nla_line[RDMA_NLDEV_ATTR_RES_IPV6_SADDR]);
> > +			if (!inet_ntop(AF_INET6, src_addr, src_addr_str,
> INET6_ADDRSTRLEN))
> > +				return -EINVAL;
> > +			dst_addr =
> mnl_attr_get_payload(nla_line[RDMA_NLDEV_ATTR_RES_IPV6_DADDR]);
> > +			if (!inet_ntop(AF_INET6, dst_addr, dst_addr_str,
> INET6_ADDRSTRLEN))
> > +				return -EINVAL;
> > +		}
> > +		if (rd_check_is_string_filtered(rd, "src-addr",
src_addr_str))
> > +			continue;
> > +		if (rd_check_is_string_filtered(rd, "dst-addr",
dst_addr_str))
> > +			continue;
> > +
> > +		src_port =
> mnl_attr_get_u16(nla_line[RDMA_NLDEV_ATTR_RES_IP_SPORT]);
> > +		dst_port =
> mnl_attr_get_u16(nla_line[RDMA_NLDEV_ATTR_RES_IP_DPORT]);
> > +		if (rd_check_is_filtered(rd, "src-port", src_port))
> > +			continue;
> > +		if (rd_check_is_filtered(rd, "dst-port", dst_port))
> > +			continue;
> > +
> 
> The same memory leaks as above, I put get_task_name() to the end of
> QP parsing code with purpose to avoid dealing with free() calls.

Sounds good.  I'll look at your qp code.

> 
> > +		if (rd->json_output)
> > +			jsonw_start_array(rd->jw);
> > +
> > +		print_link(rd, idx, name, port, nla_line);
> > +		print_lqpn(rd, lqpn);
> > +		print_qp_type(rd, type);
> > +		print_cm_id_state(rd, state);
> > +		print_ps(rd, ps);
> > +		print_dev_type(rd, dev_type);
> > +		print_transport_type(rd, transport_type);
> > +		print_pid(rd, pid);
> > +		print_comm(rd, comm, nla_line);
> > +		print_ipaddr(rd, "src-addr", src_addr_str);
> > +		print_ipport(rd, "src-port", src_port);
> > +		print_ipaddr(rd, "dst-addr", dst_addr_str);
> > +		print_ipport(rd, "dst-port", dst_port);
> 
> Does "ip tool" have standard Re presentation for addr<->port tupples?
> What about the following format src 1.1.1.1:1234 dst 2.2.2.2:6789?
> 

That looks fine for me.  I'm not sure if there's an 'ip' command that
displays ports along with addresses.  But let us go with add:port.


> > +
> > +		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
> > +			free(comm);
> > +
> > +		if (rd->json_output)
> > +			jsonw_end_array(rd->jw);
> > +		else
> > +			pr_out("\n");
> > +	}
> > +	return MNL_CB_OK;
> > +}
> > +
> >  RES_FUNC(res_no_args,	RDMA_NLDEV_CMD_RES_GET,	NULL, true);
> >
> >  static const struct
> > @@ -438,9 +712,9 @@ filters qp_valid_filters[MAX_NUMBER_OF_FILTERS]
> = {{ .name = "link",
> >  						   .is_number = false },
> >  						   { .name = "lqpn",
> >  						   .is_number = true },
> > -						   { .name = "rqpn",
> > -						   .is_number = true },
> > -						   { .name = "pid",
> > +						   { .name = "type",
> > +						   .is_number = false },
> > +						   { .name = "cm_id_state",
> >  						   .is_number = true },
> >  						   { .name = "sq-psn",
> >  						   .is_number = true },
> 
> Why did you change qp_valid_filters?

Oops...I'll fix this.

> 
> > @@ -455,11 +729,41 @@ filters
> qp_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
> >
> >  RES_FUNC(res_qp,	RDMA_NLDEV_CMD_RES_QP_GET, qp_valid_filters,
> false);
> >
> > +static const struct
> > +filters cm_id_valid_filters[MAX_NUMBER_OF_FILTERS] = {{ .name = "link",
> > +						   .is_number = false },
> > +						   { .name = "lqpn",
> > +						   .is_number = true },
> > +						   { .name = "qp-type",
> > +						   .is_number = false },
> > +						   { .name = "state",
> > +						   .is_number = false },
> > +						   { .name = "ps",
> > +						   .is_number = false },
> > +						   { .name = "dev-type",
> > +						   .is_number = false },
> > +						   { .name =
"transport-type",
> > +						   .is_number = false },
> > +						   { .name = "pid",
> > +						   .is_number = true },
> > +						   { .name = "src-addr",
> > +						   .is_number = false },
> > +						   { .name = "src-port",
> > +						   .is_number = true },
> > +						   { .name = "dst-addr",
> > +						   .is_number = false },
> > +						   { .name = "dst-port",
> > +						   .is_number = true }};
> > +
> > +RES_FUNC(res_cm_id,	RDMA_NLDEV_CMD_RES_CM_ID_GET,
> cm_id_valid_filters,
> > +	 false);
> > +
> >  static int res_show(struct rd *rd)
> >  {
> >  	const struct rd_cmd cmds[] = {
> >  		{ NULL,		res_no_args	},
> >  		{ "qp",		res_qp		},
> > +		{ "cm_id",	res_cm_id	},
> >  		{ 0 }
> >  	};
> >
> > diff --git a/rdma/utils.c b/rdma/utils.c
> > index f946016..906ca73 100644
> > --- a/rdma/utils.c
> > +++ b/rdma/utils.c
> > @@ -375,6 +375,18 @@ static const enum mnl_attr_data_type
> nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
> >  	[RDMA_NLDEV_ATTR_RES_STATE]		= MNL_TYPE_U8,
> >  	[RDMA_NLDEV_ATTR_RES_PID]		= MNL_TYPE_U32,
> >  	[RDMA_NLDEV_ATTR_RES_KERN_NAME]	=
> MNL_TYPE_NUL_STRING,
> > +	[RDMA_NLDEV_ATTR_RES_CM_ID]		= MNL_TYPE_NESTED,
> > +	[RDMA_NLDEV_ATTR_RES_CM_ID_ENTRY]	= MNL_TYPE_NESTED,
> > +	[RDMA_NLDEV_ATTR_RES_PS]		= MNL_TYPE_U32,
> > +	[RDMA_NLDEV_ATTR_RES_IPV4_SADDR]	=
> MNL_TYPE_UNSPEC,
> > +	[RDMA_NLDEV_ATTR_RES_IPV4_DADDR]	=
> MNL_TYPE_UNSPEC,
> > +	[RDMA_NLDEV_ATTR_RES_IPV6_SADDR]	=
> MNL_TYPE_UNSPEC,
> > +	[RDMA_NLDEV_ATTR_RES_IPV6_DADDR]	=
> MNL_TYPE_UNSPEC,
> > +	[RDMA_NLDEV_ATTR_RES_IP_SPORT]		= MNL_TYPE_U16,
> > +	[RDMA_NLDEV_ATTR_RES_IP_DPORT]		= MNL_TYPE_U16,
> > +	[RDMA_NLDEV_ATTR_RES_DEV_TYPE]		= MNL_TYPE_U8,
> > +	[RDMA_NLDEV_ATTR_RES_TRANSPORT_TYPE]	= MNL_TYPE_U8,
> > +	[RDMA_NLDEV_ATTR_RES_NETWORK_TYPE]	= MNL_TYPE_U8,
> >  };
> >
> >  int rd_attr_cb(const struct nlattr *attr, void *data)
> > --
> > 1.8.3.1
> >