[PATCH v1 iproute2-next 0/3] RDMA tool driver-specific resource tracking

[PATCH v1 iproute2-next 0/3] RDMA tool driver-specific resource tracking

[Steve Wise]

Hello,

This series enhances the iproute2 rdma tool to include displaying
driver-specific resource attributes.  It is the user-space part of the
kernel driver resource tracking series that has been accepted for merging
into linux-4.18 [1]

If there are no additional review comments, it can now be merged, I think.

Changes since v0/rfc:
- changed "provider" to "driver" based on kernel side changes
- updated man pages
- removed "RFC" tag

Thanks,

Steve.

[1] https://www.spinics.net/lists/linux-rdma/msg64199.html

Steve Wise (3):
  rdma: update rdma_netlink.h to get driver attrs
  rdma: print driver resource attributes
  rdma: update man pages

 man/man8/rdma-resource.8              |  29 ++++-
 man/man8/rdma.8                       |   2 +-
 rdma/include/uapi/rdma/rdma_netlink.h |  37 ++++++-
 rdma/rdma.c                           |   7 +-
 rdma/rdma.h                           |  11 ++
 rdma/res.c                            |  30 ++----
 rdma/utils.c                          | 194 ++++++++++++++++++++++++++++++++++
 7 files changed, 284 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH v1 iproute2-next 1/3] rdma: update rdma_netlink.h to get driver attrs

[Steve Wise]

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 rdma/include/uapi/rdma/rdma_netlink.h | 37 ++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
index 45474f1..40be0d8 100644
--- a/rdma/include/uapi/rdma/rdma_netlink.h
+++ b/rdma/include/uapi/rdma/rdma_netlink.h
@@ -249,10 +249,22 @@ enum rdma_nldev_command {
 	RDMA_NLDEV_NUM_OPS
 };
 
+enum {
+	RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16,
+};
+
+enum rdma_nldev_print_type {
+	RDMA_NLDEV_PRINT_TYPE_UNSPEC,
+	RDMA_NLDEV_PRINT_TYPE_HEX,
+};
+
 enum rdma_nldev_attr {
 	/* don't change the order or add anything between, this is ABI! */
 	RDMA_NLDEV_ATTR_UNSPEC,
 
+	/* Pad attribute for 64b alignment */
+	RDMA_NLDEV_ATTR_PAD = RDMA_NLDEV_ATTR_UNSPEC,
+
 	/* Identifier for ib_device */
 	RDMA_NLDEV_ATTR_DEV_INDEX,		/* u32 */
 
@@ -387,8 +399,31 @@ enum rdma_nldev_attr {
 	RDMA_NLDEV_ATTR_RES_PD_ENTRY,		/* nested table */
 	RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY,	/* u32 */
 	RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY,	/* u32 */
+	/*
+	 * driver-specific attributes.
+	 */
+	RDMA_NLDEV_ATTR_DRIVER,			/* nested table */
+	RDMA_NLDEV_ATTR_DRIVER_ENTRY,		/* nested table */
+	RDMA_NLDEV_ATTR_DRIVER_STRING,		/* string */
+	/*
+	 * u8 values from enum rdma_nldev_print_type
+	 */
+	RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE,	/* u8 */
+	RDMA_NLDEV_ATTR_DRIVER_S32,		/* s32 */
+	RDMA_NLDEV_ATTR_DRIVER_U32,		/* u32 */
+	RDMA_NLDEV_ATTR_DRIVER_S64,		/* s64 */
+	RDMA_NLDEV_ATTR_DRIVER_U64,		/* u64 */
 
-	/* Netdev information for relevant protocols, like RoCE and iWARP */
+	/*
+	 * Provides logical name and index of netdevice which is
+	 * connected to physical port. This information is relevant
+	 * for RoCE and iWARP.
+	 *
+	 * The netdevices which are associated with containers are
+	 * supposed to be exported together with GID table once it
+	 * will be exposed through the netlink. Because the
+	 * associated netdevices are properties of GIDs.
+	 */
 	RDMA_NLDEV_ATTR_NDEV_INDEX,		/* u32 */
 	RDMA_NLDEV_ATTR_NDEV_NAME,		/* string */
 
-- 
1.8.3.1

Re: [PATCH v1 iproute2-next 1/3] rdma: update rdma_netlink.h to get driver attrs

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

On Mon, May 07, 2018 at 08:53:10AM -0700, Steve Wise wrote:
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/include/uapi/rdma/rdma_netlink.h | 37 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)

Please write in commit message something like: "Based on kernel commit
....", so we will be able to track changes.

>
> diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
> index 45474f1..40be0d8 100644
> --- a/rdma/include/uapi/rdma/rdma_netlink.h
> +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> @@ -249,10 +249,22 @@ enum rdma_nldev_command {
>  	RDMA_NLDEV_NUM_OPS
>  };
>
> +enum {
> +	RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16,
> +};
> +
> +enum rdma_nldev_print_type {
> +	RDMA_NLDEV_PRINT_TYPE_UNSPEC,
> +	RDMA_NLDEV_PRINT_TYPE_HEX,
> +};
> +
>  enum rdma_nldev_attr {
>  	/* don't change the order or add anything between, this is ABI! */
>  	RDMA_NLDEV_ATTR_UNSPEC,
>
> +	/* Pad attribute for 64b alignment */
> +	RDMA_NLDEV_ATTR_PAD = RDMA_NLDEV_ATTR_UNSPEC,
> +
>  	/* Identifier for ib_device */
>  	RDMA_NLDEV_ATTR_DEV_INDEX,		/* u32 */
>
> @@ -387,8 +399,31 @@ enum rdma_nldev_attr {
>  	RDMA_NLDEV_ATTR_RES_PD_ENTRY,		/* nested table */
>  	RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY,	/* u32 */
>  	RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY,	/* u32 */
> +	/*
> +	 * driver-specific attributes.
> +	 */
> +	RDMA_NLDEV_ATTR_DRIVER,			/* nested table */
> +	RDMA_NLDEV_ATTR_DRIVER_ENTRY,		/* nested table */
> +	RDMA_NLDEV_ATTR_DRIVER_STRING,		/* string */
> +	/*
> +	 * u8 values from enum rdma_nldev_print_type
> +	 */
> +	RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE,	/* u8 */
> +	RDMA_NLDEV_ATTR_DRIVER_S32,		/* s32 */
> +	RDMA_NLDEV_ATTR_DRIVER_U32,		/* u32 */
> +	RDMA_NLDEV_ATTR_DRIVER_S64,		/* s64 */
> +	RDMA_NLDEV_ATTR_DRIVER_U64,		/* u64 */
>
> -	/* Netdev information for relevant protocols, like RoCE and iWARP */
> +	/*
> +	 * Provides logical name and index of netdevice which is
> +	 * connected to physical port. This information is relevant
> +	 * for RoCE and iWARP.
> +	 *
> +	 * The netdevices which are associated with containers are
> +	 * supposed to be exported together with GID table once it
> +	 * will be exposed through the netlink. Because the
> +	 * associated netdevices are properties of GIDs.
> +	 */
>  	RDMA_NLDEV_ATTR_NDEV_INDEX,		/* u32 */
>  	RDMA_NLDEV_ATTR_NDEV_NAME,		/* string */
>
> --
> 1.8.3.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v1 iproute2-next 1/3] rdma: update rdma_netlink.h to get driver attrs

[Steve Wise]

On 5/13/2018 8:15 AM, Leon Romanovsky wrote:
> On Mon, May 07, 2018 at 08:53:10AM -0700, Steve Wise wrote:
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/include/uapi/rdma/rdma_netlink.h | 37 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
> Please write in commit message something like: "Based on kernel commit
> ....", so we will be able to track changes.

Sure.

Thanks,

Steve.

[PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

This enhancement allows printing rdma device-specific state, if provided
by the kernel.  This is done in a generic manner, so rdma tool doesn't
need to know about the details of every type of rdma device.

Driver attributes for a rdma resource are in the form of <key,
[print_type], value> tuples, where the key is a string and the value can
be any supported driver attribute.  The print_type attribute, if present,
provides a print format to use vs the standard print format for the type.
For example, the default print type for a PROVIDER_S32 value is "%d ",
but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.

Driver resources are only printed when the -dd flag is present.
If -p is present, then the output is formatted to not exceed 80 columns,
otherwise it is printed as a single row to be grep/awk friendly.

Example output:

# rdma resource show qp lqpn 1028 -dd -p
link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
    sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
    size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
    rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 rdma/rdma.c  |   7 ++-
 rdma/rdma.h  |  11 ++++
 rdma/res.c   |  30 +++------
 rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 221 insertions(+), 21 deletions(-)

diff --git a/rdma/rdma.c b/rdma/rdma.c
index b43e538..0155627 100644
--- a/rdma/rdma.c
+++ b/rdma/rdma.c
@@ -132,6 +132,7 @@ int main(int argc, char **argv)
 	const char *batch_file = NULL;
 	bool pretty_output = false;
 	bool show_details = false;
+	bool show_driver_details = false;
 	bool json_output = false;
 	bool force = false;
 	char *filename;
@@ -152,7 +153,10 @@ int main(int argc, char **argv)
 			pretty_output = true;
 			break;
 		case 'd':
-			show_details = true;
+			if (show_details)
+				show_driver_details = true;
+			else
+				show_details = true;
 			break;
 		case 'j':
 			json_output = true;
@@ -180,6 +184,7 @@ int main(int argc, char **argv)
 	argv += optind;
 
 	rd.show_details = show_details;
+	rd.show_driver_details = show_driver_details;
 	rd.json_output = json_output;
 	rd.pretty_output = pretty_output;
 
diff --git a/rdma/rdma.h b/rdma/rdma.h
index 1908fc4..fcaf9e6 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -55,6 +55,7 @@ struct rd {
 	char **argv;
 	char *filename;
 	bool show_details;
+	bool show_driver_details;
 	struct list_head dev_map_list;
 	uint32_t dev_idx;
 	uint32_t port_idx;
@@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
 void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags);
 int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data);
 int rd_attr_cb(const struct nlattr *attr, void *data);
+int rd_attr_check(const struct nlattr *attr, int *typep);
+
+/*
+ * Print helpers
+ */
+void print_driver_table(struct rd *rd, struct nlattr *tb);
+void newline(struct rd *rd);
+void newline_indent(struct rd *rd);
+#define MAX_LINE_LENGTH 80
+
 #endif /* _RDMA_TOOL_H_ */
diff --git a/rdma/res.c b/rdma/res.c
index 1a0aab6..074b992 100644
--- a/rdma/res.c
+++ b/rdma/res.c
@@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data)
 		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
 			free(comm);
 
-		if (rd->json_output)
-			jsonw_end_array(rd->jw);
-		else
-			pr_out("\n");
+		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
+		newline(rd);
 	}
 	return MNL_CB_OK;
 }
@@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data)
 		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
 			free(comm);
 
-		if (rd->json_output)
-			jsonw_end_array(rd->jw);
-		else
-			pr_out("\n");
+		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
+		newline(rd);
 	}
 	return MNL_CB_OK;
 }
@@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data)
 		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
 			free(comm);
 
-		if (rd->json_output)
-			jsonw_end_array(rd->jw);
-		else
-			pr_out("\n");
+		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
+		newline(rd);
 	}
 	return MNL_CB_OK;
 }
@@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data)
 		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
 			free(comm);
 
-		if (rd->json_output)
-			jsonw_end_array(rd->jw);
-		else
-			pr_out("\n");
+		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
+		newline(rd);
 	}
 	return MNL_CB_OK;
 }
@@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data)
 		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
 			free(comm);
 
-		if (rd->json_output)
-			jsonw_end_array(rd->jw);
-		else
-			pr_out("\n");
+		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
+		newline(rd);
 	}
 	return MNL_CB_OK;
 }
diff --git a/rdma/utils.c b/rdma/utils.c
index 49c967f..452fe92 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -11,6 +11,7 @@
 
 #include "rdma.h"
 #include <ctype.h>
+#include <inttypes.h>
 
 int rd_argc(struct rd *rd)
 {
@@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
 	[RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64,
 	[RDMA_NLDEV_ATTR_NDEV_INDEX]		= MNL_TYPE_U32,
 	[RDMA_NLDEV_ATTR_NDEV_NAME]		= MNL_TYPE_NUL_STRING,
+	[RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED,
+	[RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED,
+	[RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING,
+	[RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8,
+	[RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32,
+	[RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32,
+	[RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64,
+	[RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64,
 };
 
+int rd_attr_check(const struct nlattr *attr, int *typep)
+{
+	int type;
+
+	if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0)
+		return MNL_CB_ERROR;
+
+	type = mnl_attr_get_type(attr);
+
+	if (mnl_attr_validate(attr, nldev_policy[type]) < 0)
+		return MNL_CB_ERROR;
+
+	*typep = nldev_policy[type];
+	return MNL_CB_OK;
+}
+
 int rd_attr_cb(const struct nlattr *attr, void *data)
 {
 	const struct nlattr **tb = data;
@@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index)
 	free(dev_name);
 	return dev_map;
 }
+
+#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK)
+
+void newline(struct rd *rd)
+{
+	if (rd->json_output)
+		jsonw_end_array(rd->jw);
+	else
+		pr_out("\n");
+}
+
+void newline_indent(struct rd *rd)
+{
+	newline(rd);
+	if (!rd->json_output)
+		pr_out("    ");
+}
+
+static int print_driver_string(struct rd *rd, const char *key_str,
+				 const char *val_str)
+{
+	if (rd->json_output) {
+		jsonw_string_field(rd->jw, key_str, val_str);
+		return 0;
+	} else {
+		return pr_out("%s %s ", key_str, val_str);
+	}
+}
+
+static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val,
+			      enum rdma_nldev_print_type print_type)
+{
+	if (rd->json_output) {
+		jsonw_int_field(rd->jw, key_str, val);
+		return 0;
+	}
+	switch (print_type) {
+	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
+		return pr_out("%s %d ", key_str, val);
+	case RDMA_NLDEV_PRINT_TYPE_HEX:
+		return pr_out("%s 0x%x ", key_str, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val,
+			      enum rdma_nldev_print_type print_type)
+{
+	if (rd->json_output) {
+		jsonw_int_field(rd->jw, key_str, val);
+		return 0;
+	}
+	switch (print_type) {
+	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
+		return pr_out("%s %u ", key_str, val);
+	case RDMA_NLDEV_PRINT_TYPE_HEX:
+		return pr_out("%s 0x%x ", key_str, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val,
+			      enum rdma_nldev_print_type print_type)
+{
+	if (rd->json_output) {
+		jsonw_int_field(rd->jw, key_str, val);
+		return 0;
+	}
+	switch (print_type) {
+	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
+		return pr_out("%s %" PRId64 " ", key_str, val);
+	case RDMA_NLDEV_PRINT_TYPE_HEX:
+		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val,
+			      enum rdma_nldev_print_type print_type)
+{
+	if (rd->json_output) {
+		jsonw_int_field(rd->jw, key_str, val);
+		return 0;
+	}
+	switch (print_type) {
+	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
+		return pr_out("%s %" PRIu64 " ", key_str, val);
+	case RDMA_NLDEV_PRINT_TYPE_HEX:
+		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int print_driver_entry(struct rd *rd, struct nlattr *key_attr,
+				struct nlattr *val_attr,
+				enum rdma_nldev_print_type print_type)
+{
+	const char *key_str = mnl_attr_get_str(key_attr);
+	int attr_type = nla_type(val_attr);
+
+	switch (attr_type) {
+	case RDMA_NLDEV_ATTR_DRIVER_STRING:
+		return print_driver_string(rd, key_str,
+				mnl_attr_get_str(val_attr));
+	case RDMA_NLDEV_ATTR_DRIVER_S32:
+		return print_driver_s32(rd, key_str,
+				mnl_attr_get_u32(val_attr), print_type);
+	case RDMA_NLDEV_ATTR_DRIVER_U32:
+		return print_driver_u32(rd, key_str,
+				mnl_attr_get_u32(val_attr), print_type);
+	case RDMA_NLDEV_ATTR_DRIVER_S64:
+		return print_driver_s64(rd, key_str,
+				mnl_attr_get_u64(val_attr), print_type);
+	case RDMA_NLDEV_ATTR_DRIVER_U64:
+		return print_driver_u64(rd, key_str,
+				mnl_attr_get_u64(val_attr), print_type);
+	}
+	return -EINVAL;
+}
+
+void print_driver_table(struct rd *rd, struct nlattr *tb)
+{
+	int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
+	struct nlattr *tb_entry, *key = NULL, *val;
+	int type, cc = 0;
+
+	if (!rd->show_driver_details || !tb)
+		return;
+
+	if (rd->pretty_output)
+		newline_indent(rd);
+
+	/*
+	 * Driver attrs are tuples of {key, [print-type], value}.
+	 * The key must be a string.  If print-type is present, it 
+	 * defines an alternate printf format type vs the native format
+	 * for the attribute.  And the value can be any available
+	 * driver type.
+	 */
+	mnl_attr_for_each_nested(tb_entry, tb) {
+
+		if (cc > MAX_LINE_LENGTH) {
+			if (rd->pretty_output)
+				newline_indent(rd);
+			cc = 0;
+		}
+		if (rd_attr_check(tb_entry, &type) != MNL_CB_OK)
+			return;
+		if (!key) {
+			if (type != MNL_TYPE_NUL_STRING)
+				return;
+			key = tb_entry;
+		} else if (type == MNL_TYPE_U8) {
+			print_type = mnl_attr_get_u8(tb_entry);
+		} else {
+			val = tb_entry;
+			cc += print_driver_entry(rd, key, val, print_type);
+			if (cc < 0)
+				return;
+			print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
+			key = NULL;
+		}
+	}
+	return;
+}
-- 
1.8.3.1

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[David Ahern]

On 5/7/18 9:53 AM, Steve Wise wrote:
> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
>  			pretty_output = true;
>  			break;
>  		case 'd':
> -			show_details = true;
> +			if (show_details)
> +				show_driver_details = true;
> +			else
> +				show_details = true;
>  			break;
>  		case 'j':
>  			json_output = true;

The above change should be reflected in the man page.

Also, the set needs to be respun after I merged master where Stephen
brought in updates to the uapi files.

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

On 5/9/2018 11:08 PM, David Ahern wrote:
> On 5/7/18 9:53 AM, Steve Wise wrote:
>> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
>>  			pretty_output = true;
>>  			break;
>>  		case 'd':
>> -			show_details = true;
>> +			if (show_details)
>> +				show_driver_details = true;
>> +			else
>> +				show_details = true;
>>  			break;
>>  		case 'j':
>>  			json_output = true;
> The above change should be reflected in the man page.

I did mention it in the man page:

       -d, --details
              Output detailed information.  Adding a second -d includes
driver-specific details.

But I wasn't sure how to show it in the syntax.  Maybe this?

 OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] }


> Also, the set needs to be respun after I merged master where Stephen
> brought in updates to the uapi files.

Will do.  Thanks for reviewing.

Steve.

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[David Ahern]

On 5/10/18 8:19 AM, Steve Wise wrote:
> 
> On 5/9/2018 11:08 PM, David Ahern wrote:
>> On 5/7/18 9:53 AM, Steve Wise wrote:
>>> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
>>>  			pretty_output = true;
>>>  			break;
>>>  		case 'd':
>>> -			show_details = true;
>>> +			if (show_details)
>>> +				show_driver_details = true;
>>> +			else
>>> +				show_details = true;
>>>  			break;
>>>  		case 'j':
>>>  			json_output = true;
>> The above change should be reflected in the man page.
> 
> I did mention it in the man page:
> 
>        -d, --details
>               Output detailed information.  Adding a second -d includes
> driver-specific details.
> 
> But I wasn't sure how to show it in the syntax.  Maybe this?
> 
>  OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] }

I should have read the second patch before commenting. Didn't it have
first -d = details, a second -d = driver details? That should be fine.

> 
> 
>> Also, the set needs to be respun after I merged master where Stephen
>> brought in updates to the uapi files.
> 
> Will do.  Thanks for reviewing.
> 
> Steve.
> 

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Thu, May 10, 2018 at 08:20:51AM -0600, David Ahern wrote:
> On 5/10/18 8:19 AM, Steve Wise wrote:
> >
> > On 5/9/2018 11:08 PM, David Ahern wrote:
> >> On 5/7/18 9:53 AM, Steve Wise wrote:
> >>> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
> >>>  			pretty_output = true;
> >>>  			break;
> >>>  		case 'd':
> >>> -			show_details = true;
> >>> +			if (show_details)
> >>> +				show_driver_details = true;
> >>> +			else
> >>> +				show_details = true;
> >>>  			break;
> >>>  		case 'j':
> >>>  			json_output = true;
> >> The above change should be reflected in the man page.
> >
> > I did mention it in the man page:
> >
> >        -d, --details
> >               Output detailed information.  Adding a second -d includes
> > driver-specific details.
> >
> > But I wasn't sure how to show it in the syntax.  Maybe this?
> >
> >  OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] }
>
> I should have read the second patch before commenting. Didn't it have
> first -d = details, a second -d = driver details? That should be fine.

Yes, our idea is to require "-dd" to print such driver specific
information. The level of nesting is:
 * No arguments -> info usable for most of the users
 * -d - pre-parsed flags and rarely used information.
 * -dd - very detailed output, can be very specific to device.

Thanks

>
> >
> >
> >> Also, the set needs to be respun after I merged master where Stephen
> >> brought in updates to the uapi files.
> >
> > Will do.  Thanks for reviewing.
> >
> > Steve.
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 11971 bytes --]

On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> This enhancement allows printing rdma device-specific state, if provided
> by the kernel.  This is done in a generic manner, so rdma tool doesn't

Double space between "." and "This".

> need to know about the details of every type of rdma device.
>
> Driver attributes for a rdma resource are in the form of <key,
> [print_type], value> tuples, where the key is a string and the value can
> be any supported driver attribute.  The print_type attribute, if present,

ditto

> provides a print format to use vs the standard print format for the type.
> For example, the default print type for a PROVIDER_S32 value is "%d ",
> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.
>
> Driver resources are only printed when the -dd flag is present.
> If -p is present, then the output is formatted to not exceed 80 columns,
> otherwise it is printed as a single row to be grep/awk friendly.
>
> Example output:
>
> # rdma resource show qp lqpn 1028 -dd -p
> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0
>
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/rdma.c  |   7 ++-
>  rdma/rdma.h  |  11 ++++
>  rdma/res.c   |  30 +++------
>  rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 221 insertions(+), 21 deletions(-)
>
> diff --git a/rdma/rdma.c b/rdma/rdma.c
> index b43e538..0155627 100644
> --- a/rdma/rdma.c
> +++ b/rdma/rdma.c
> @@ -132,6 +132,7 @@ int main(int argc, char **argv)
>  	const char *batch_file = NULL;
>  	bool pretty_output = false;
>  	bool show_details = false;
> +	bool show_driver_details = false;

Reversed Christmas tree please.

>  	bool json_output = false;
>  	bool force = false;
>  	char *filename;
> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
>  			pretty_output = true;
>  			break;
>  		case 'd':
> -			show_details = true;
> +			if (show_details)
> +				show_driver_details = true;
> +			else
> +				show_details = true;
>  			break;
>  		case 'j':
>  			json_output = true;
> @@ -180,6 +184,7 @@ int main(int argc, char **argv)
>  	argv += optind;
>
>  	rd.show_details = show_details;
> +	rd.show_driver_details = show_driver_details;
>  	rd.json_output = json_output;
>  	rd.pretty_output = pretty_output;
>
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 1908fc4..fcaf9e6 100644
> --- a/rdma/rdma.h
> +++ b/rdma/rdma.h
> @@ -55,6 +55,7 @@ struct rd {
>  	char **argv;
>  	char *filename;
>  	bool show_details;
> +	bool show_driver_details;
>  	struct list_head dev_map_list;
>  	uint32_t dev_idx;
>  	uint32_t port_idx;
> @@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>  void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags);
>  int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data);
>  int rd_attr_cb(const struct nlattr *attr, void *data);
> +int rd_attr_check(const struct nlattr *attr, int *typep);
> +
> +/*
> + * Print helpers
> + */
> +void print_driver_table(struct rd *rd, struct nlattr *tb);
> +void newline(struct rd *rd);
> +void newline_indent(struct rd *rd);
> +#define MAX_LINE_LENGTH 80
> +
>  #endif /* _RDMA_TOOL_H_ */
> diff --git a/rdma/res.c b/rdma/res.c
> index 1a0aab6..074b992 100644
> --- a/rdma/res.c
> +++ b/rdma/res.c
> @@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data)
>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>  			free(comm);
>
> -		if (rd->json_output)
> -			jsonw_end_array(rd->jw);
> -		else
> -			pr_out("\n");
> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
> +		newline(rd);
>  	}
>  	return MNL_CB_OK;
>  }
> @@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data)
>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>  			free(comm);
>
> -		if (rd->json_output)
> -			jsonw_end_array(rd->jw);
> -		else
> -			pr_out("\n");
> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
> +		newline(rd);
>  	}
>  	return MNL_CB_OK;
>  }
> @@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data)
>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>  			free(comm);
>
> -		if (rd->json_output)
> -			jsonw_end_array(rd->jw);
> -		else
> -			pr_out("\n");
> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
> +		newline(rd);
>  	}
>  	return MNL_CB_OK;
>  }
> @@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data)
>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>  			free(comm);
>
> -		if (rd->json_output)
> -			jsonw_end_array(rd->jw);
> -		else
> -			pr_out("\n");
> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
> +		newline(rd);
>  	}
>  	return MNL_CB_OK;
>  }
> @@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data)
>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>  			free(comm);
>
> -		if (rd->json_output)
> -			jsonw_end_array(rd->jw);
> -		else
> -			pr_out("\n");
> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
> +		newline(rd);
>  	}
>  	return MNL_CB_OK;
>  }
> diff --git a/rdma/utils.c b/rdma/utils.c
> index 49c967f..452fe92 100644
> --- a/rdma/utils.c
> +++ b/rdma/utils.c
> @@ -11,6 +11,7 @@
>
>  #include "rdma.h"
>  #include <ctype.h>
> +#include <inttypes.h>
>
>  int rd_argc(struct rd *rd)
>  {
> @@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
>  	[RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64,
>  	[RDMA_NLDEV_ATTR_NDEV_INDEX]		= MNL_TYPE_U32,
>  	[RDMA_NLDEV_ATTR_NDEV_NAME]		= MNL_TYPE_NUL_STRING,
> +	[RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED,
> +	[RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED,
> +	[RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING,
> +	[RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8,
> +	[RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32,
> +	[RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32,
> +	[RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64,
> +	[RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64,
>  };
>
> +int rd_attr_check(const struct nlattr *attr, int *typep)
> +{
> +	int type;
> +
> +	if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0)
> +		return MNL_CB_ERROR;
> +
> +	type = mnl_attr_get_type(attr);
> +
> +	if (mnl_attr_validate(attr, nldev_policy[type]) < 0)
> +		return MNL_CB_ERROR;
> +
> +	*typep = nldev_policy[type];
> +	return MNL_CB_OK;
> +}
> +
>  int rd_attr_cb(const struct nlattr *attr, void *data)
>  {
>  	const struct nlattr **tb = data;
> @@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index)
>  	free(dev_name);
>  	return dev_map;
>  }
> +
> +#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK)
> +
> +void newline(struct rd *rd)
> +{
> +	if (rd->json_output)
> +		jsonw_end_array(rd->jw);
> +	else
> +		pr_out("\n");
> +}
> +
> +void newline_indent(struct rd *rd)
> +{
> +	newline(rd);
> +	if (!rd->json_output)
> +		pr_out("    ");
> +}
> +
> +static int print_driver_string(struct rd *rd, const char *key_str,
> +				 const char *val_str)
> +{
> +	if (rd->json_output) {
> +		jsonw_string_field(rd->jw, key_str, val_str);
> +		return 0;
> +	} else {
> +		return pr_out("%s %s ", key_str, val_str);
> +	}
> +}
> +
> +static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val,
> +			      enum rdma_nldev_print_type print_type)
> +{
> +	if (rd->json_output) {
> +		jsonw_int_field(rd->jw, key_str, val);
> +		return 0;
> +	}
> +	switch (print_type) {
> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
> +		return pr_out("%s %d ", key_str, val);
> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
> +		return pr_out("%s 0x%x ", key_str, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val,
> +			      enum rdma_nldev_print_type print_type)
> +{
> +	if (rd->json_output) {
> +		jsonw_int_field(rd->jw, key_str, val);
> +		return 0;
> +	}
> +	switch (print_type) {
> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
> +		return pr_out("%s %u ", key_str, val);
> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
> +		return pr_out("%s 0x%x ", key_str, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val,
> +			      enum rdma_nldev_print_type print_type)
> +{
> +	if (rd->json_output) {
> +		jsonw_int_field(rd->jw, key_str, val);
> +		return 0;
> +	}
> +	switch (print_type) {
> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
> +		return pr_out("%s %" PRId64 " ", key_str, val);
> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
> +		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val,
> +			      enum rdma_nldev_print_type print_type)
> +{
> +	if (rd->json_output) {
> +		jsonw_int_field(rd->jw, key_str, val);
> +		return 0;
> +	}
> +	switch (print_type) {
> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
> +		return pr_out("%s %" PRIu64 " ", key_str, val);
> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
> +		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int print_driver_entry(struct rd *rd, struct nlattr *key_attr,
> +				struct nlattr *val_attr,
> +				enum rdma_nldev_print_type print_type)
> +{
> +	const char *key_str = mnl_attr_get_str(key_attr);
> +	int attr_type = nla_type(val_attr);
> +
> +	switch (attr_type) {
> +	case RDMA_NLDEV_ATTR_DRIVER_STRING:
> +		return print_driver_string(rd, key_str,
> +				mnl_attr_get_str(val_attr));
> +	case RDMA_NLDEV_ATTR_DRIVER_S32:
> +		return print_driver_s32(rd, key_str,
> +				mnl_attr_get_u32(val_attr), print_type);
> +	case RDMA_NLDEV_ATTR_DRIVER_U32:
> +		return print_driver_u32(rd, key_str,
> +				mnl_attr_get_u32(val_attr), print_type);
> +	case RDMA_NLDEV_ATTR_DRIVER_S64:
> +		return print_driver_s64(rd, key_str,
> +				mnl_attr_get_u64(val_attr), print_type);
> +	case RDMA_NLDEV_ATTR_DRIVER_U64:
> +		return print_driver_u64(rd, key_str,
> +				mnl_attr_get_u64(val_attr), print_type);
> +	}
> +	return -EINVAL;
> +}
> +
> +void print_driver_table(struct rd *rd, struct nlattr *tb)
> +{
> +	int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
> +	struct nlattr *tb_entry, *key = NULL, *val;
> +	int type, cc = 0;
> +
> +	if (!rd->show_driver_details || !tb)
> +		return;
> +
> +	if (rd->pretty_output)
> +		newline_indent(rd);
> +
> +	/*
> +	 * Driver attrs are tuples of {key, [print-type], value}.
> +	 * The key must be a string.  If print-type is present, it
> +	 * defines an alternate printf format type vs the native format
> +	 * for the attribute.  And the value can be any available
> +	 * driver type.
> +	 */
> +	mnl_attr_for_each_nested(tb_entry, tb) {
> +
> +		if (cc > MAX_LINE_LENGTH) {
> +			if (rd->pretty_output)
> +				newline_indent(rd);
> +			cc = 0;
> +		}
> +		if (rd_attr_check(tb_entry, &type) != MNL_CB_OK)
> +			return;
> +		if (!key) {
> +			if (type != MNL_TYPE_NUL_STRING)
> +				return;
> +			key = tb_entry;
> +		} else if (type == MNL_TYPE_U8) {
> +			print_type = mnl_attr_get_u8(tb_entry);
> +		} else {
> +			val = tb_entry;
> +			cc += print_driver_entry(rd, key, val, print_type);

I stopped to read here, because of two problems:
1. print_driver_entry can return negative number, so unclear to me what
will be the final result of "cc += ..".
2. The netlink design is to ignore unknown attributes and not return
error. It allows to use new kernels with old applications.

> +			if (cc < 0)
> +				return;
> +			print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
> +			key = NULL;
> +		}
> +	}
> +	return;
> +}
> --
> 1.8.3.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

On 5/13/2018 8:24 AM, Leon Romanovsky wrote:
> On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
>> This enhancement allows printing rdma device-specific state, if provided
>> by the kernel.  This is done in a generic manner, so rdma tool doesn't
> Double space between "." and "This".
>
>> need to know about the details of every type of rdma device.
>>
>> Driver attributes for a rdma resource are in the form of <key,
>> [print_type], value> tuples, where the key is a string and the value can
>> be any supported driver attribute.  The print_type attribute, if present,
> ditto

I'll fix these.

>> provides a print format to use vs the standard print format for the type.
>> For example, the default print type for a PROVIDER_S32 value is "%d ",
>> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.
>>
>> Driver resources are only printed when the -dd flag is present.
>> If -p is present, then the output is formatted to not exceed 80 columns,
>> otherwise it is printed as a single row to be grep/awk friendly.
>>
>> Example output:
>>
>> # rdma resource show qp lqpn 1028 -dd -p
>> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
>>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
>>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
>>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/rdma.c  |   7 ++-
>>  rdma/rdma.h  |  11 ++++
>>  rdma/res.c   |  30 +++------
>>  rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 221 insertions(+), 21 deletions(-)
>>
>> diff --git a/rdma/rdma.c b/rdma/rdma.c
>> index b43e538..0155627 100644
>> --- a/rdma/rdma.c
>> +++ b/rdma/rdma.c
>> @@ -132,6 +132,7 @@ int main(int argc, char **argv)
>>  	const char *batch_file = NULL;
>>  	bool pretty_output = false;
>>  	bool show_details = false;
>> +	bool show_driver_details = false;
> Reversed Christmas tree please.

Sure. 

>>  	bool json_output = false;
>>  	bool force = false;
>>  	char *filename;
>> @@ -152,7 +153,10 @@ int main(int argc, char **argv)
>>  			pretty_output = true;
>>  			break;
>>  		case 'd':
>> -			show_details = true;
>> +			if (show_details)
>> +				show_driver_details = true;
>> +			else
>> +				show_details = true;
>>  			break;
>>  		case 'j':
>>  			json_output = true;
>> @@ -180,6 +184,7 @@ int main(int argc, char **argv)
>>  	argv += optind;
>>
>>  	rd.show_details = show_details;
>> +	rd.show_driver_details = show_driver_details;
>>  	rd.json_output = json_output;
>>  	rd.pretty_output = pretty_output;
>>
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 1908fc4..fcaf9e6 100644
>> --- a/rdma/rdma.h
>> +++ b/rdma/rdma.h
>> @@ -55,6 +55,7 @@ struct rd {
>>  	char **argv;
>>  	char *filename;
>>  	bool show_details;
>> +	bool show_driver_details;
>>  	struct list_head dev_map_list;
>>  	uint32_t dev_idx;
>>  	uint32_t port_idx;
>> @@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>>  void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags);
>>  int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data);
>>  int rd_attr_cb(const struct nlattr *attr, void *data);
>> +int rd_attr_check(const struct nlattr *attr, int *typep);
>> +
>> +/*
>> + * Print helpers
>> + */
>> +void print_driver_table(struct rd *rd, struct nlattr *tb);
>> +void newline(struct rd *rd);
>> +void newline_indent(struct rd *rd);
>> +#define MAX_LINE_LENGTH 80
>> +
>>  #endif /* _RDMA_TOOL_H_ */
>> diff --git a/rdma/res.c b/rdma/res.c
>> index 1a0aab6..074b992 100644
>> --- a/rdma/res.c
>> +++ b/rdma/res.c
>> @@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>>  			free(comm);
>>
>> -		if (rd->json_output)
>> -			jsonw_end_array(rd->jw);
>> -		else
>> -			pr_out("\n");
>> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
>> +		newline(rd);
>>  	}
>>  	return MNL_CB_OK;
>>  }
>> @@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>>  			free(comm);
>>
>> -		if (rd->json_output)
>> -			jsonw_end_array(rd->jw);
>> -		else
>> -			pr_out("\n");
>> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
>> +		newline(rd);
>>  	}
>>  	return MNL_CB_OK;
>>  }
>> @@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>>  			free(comm);
>>
>> -		if (rd->json_output)
>> -			jsonw_end_array(rd->jw);
>> -		else
>> -			pr_out("\n");
>> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
>> +		newline(rd);
>>  	}
>>  	return MNL_CB_OK;
>>  }
>> @@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>>  			free(comm);
>>
>> -		if (rd->json_output)
>> -			jsonw_end_array(rd->jw);
>> -		else
>> -			pr_out("\n");
>> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
>> +		newline(rd);
>>  	}
>>  	return MNL_CB_OK;
>>  }
>> @@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  		if (nla_line[RDMA_NLDEV_ATTR_RES_PID])
>>  			free(comm);
>>
>> -		if (rd->json_output)
>> -			jsonw_end_array(rd->jw);
>> -		else
>> -			pr_out("\n");
>> +		print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]);
>> +		newline(rd);
>>  	}
>>  	return MNL_CB_OK;
>>  }
>> diff --git a/rdma/utils.c b/rdma/utils.c
>> index 49c967f..452fe92 100644
>> --- a/rdma/utils.c
>> +++ b/rdma/utils.c
>> @@ -11,6 +11,7 @@
>>
>>  #include "rdma.h"
>>  #include <ctype.h>
>> +#include <inttypes.h>
>>
>>  int rd_argc(struct rd *rd)
>>  {
>> @@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
>>  	[RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64,
>>  	[RDMA_NLDEV_ATTR_NDEV_INDEX]		= MNL_TYPE_U32,
>>  	[RDMA_NLDEV_ATTR_NDEV_NAME]		= MNL_TYPE_NUL_STRING,
>> +	[RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED,
>> +	[RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED,
>> +	[RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING,
>> +	[RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8,
>> +	[RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32,
>> +	[RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32,
>> +	[RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64,
>> +	[RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64,
>>  };
>>
>> +int rd_attr_check(const struct nlattr *attr, int *typep)
>> +{
>> +	int type;
>> +
>> +	if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0)
>> +		return MNL_CB_ERROR;
>> +
>> +	type = mnl_attr_get_type(attr);
>> +
>> +	if (mnl_attr_validate(attr, nldev_policy[type]) < 0)
>> +		return MNL_CB_ERROR;
>> +
>> +	*typep = nldev_policy[type];
>> +	return MNL_CB_OK;
>> +}
>> +
>>  int rd_attr_cb(const struct nlattr *attr, void *data)
>>  {
>>  	const struct nlattr **tb = data;
>> @@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index)
>>  	free(dev_name);
>>  	return dev_map;
>>  }
>> +
>> +#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK)
>> +
>> +void newline(struct rd *rd)
>> +{
>> +	if (rd->json_output)
>> +		jsonw_end_array(rd->jw);
>> +	else
>> +		pr_out("\n");
>> +}
>> +
>> +void newline_indent(struct rd *rd)
>> +{
>> +	newline(rd);
>> +	if (!rd->json_output)
>> +		pr_out("    ");
>> +}
>> +
>> +static int print_driver_string(struct rd *rd, const char *key_str,
>> +				 const char *val_str)
>> +{
>> +	if (rd->json_output) {
>> +		jsonw_string_field(rd->jw, key_str, val_str);
>> +		return 0;
>> +	} else {
>> +		return pr_out("%s %s ", key_str, val_str);
>> +	}
>> +}
>> +
>> +static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val,
>> +			      enum rdma_nldev_print_type print_type)
>> +{
>> +	if (rd->json_output) {
>> +		jsonw_int_field(rd->jw, key_str, val);
>> +		return 0;
>> +	}
>> +	switch (print_type) {
>> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
>> +		return pr_out("%s %d ", key_str, val);
>> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
>> +		return pr_out("%s 0x%x ", key_str, val);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val,
>> +			      enum rdma_nldev_print_type print_type)
>> +{
>> +	if (rd->json_output) {
>> +		jsonw_int_field(rd->jw, key_str, val);
>> +		return 0;
>> +	}
>> +	switch (print_type) {
>> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
>> +		return pr_out("%s %u ", key_str, val);
>> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
>> +		return pr_out("%s 0x%x ", key_str, val);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val,
>> +			      enum rdma_nldev_print_type print_type)
>> +{
>> +	if (rd->json_output) {
>> +		jsonw_int_field(rd->jw, key_str, val);
>> +		return 0;
>> +	}
>> +	switch (print_type) {
>> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
>> +		return pr_out("%s %" PRId64 " ", key_str, val);
>> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
>> +		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val,
>> +			      enum rdma_nldev_print_type print_type)
>> +{
>> +	if (rd->json_output) {
>> +		jsonw_int_field(rd->jw, key_str, val);
>> +		return 0;
>> +	}
>> +	switch (print_type) {
>> +	case RDMA_NLDEV_PRINT_TYPE_UNSPEC:
>> +		return pr_out("%s %" PRIu64 " ", key_str, val);
>> +	case RDMA_NLDEV_PRINT_TYPE_HEX:
>> +		return pr_out("%s 0x%" PRIx64 " ", key_str, val);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int print_driver_entry(struct rd *rd, struct nlattr *key_attr,
>> +				struct nlattr *val_attr,
>> +				enum rdma_nldev_print_type print_type)
>> +{
>> +	const char *key_str = mnl_attr_get_str(key_attr);
>> +	int attr_type = nla_type(val_attr);
>> +
>> +	switch (attr_type) {
>> +	case RDMA_NLDEV_ATTR_DRIVER_STRING:
>> +		return print_driver_string(rd, key_str,
>> +				mnl_attr_get_str(val_attr));
>> +	case RDMA_NLDEV_ATTR_DRIVER_S32:
>> +		return print_driver_s32(rd, key_str,
>> +				mnl_attr_get_u32(val_attr), print_type);
>> +	case RDMA_NLDEV_ATTR_DRIVER_U32:
>> +		return print_driver_u32(rd, key_str,
>> +				mnl_attr_get_u32(val_attr), print_type);
>> +	case RDMA_NLDEV_ATTR_DRIVER_S64:
>> +		return print_driver_s64(rd, key_str,
>> +				mnl_attr_get_u64(val_attr), print_type);
>> +	case RDMA_NLDEV_ATTR_DRIVER_U64:
>> +		return print_driver_u64(rd, key_str,
>> +				mnl_attr_get_u64(val_attr), print_type);
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +void print_driver_table(struct rd *rd, struct nlattr *tb)
>> +{
>> +	int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
>> +	struct nlattr *tb_entry, *key = NULL, *val;
>> +	int type, cc = 0;
>> +
>> +	if (!rd->show_driver_details || !tb)
>> +		return;
>> +
>> +	if (rd->pretty_output)
>> +		newline_indent(rd);
>> +
>> +	/*
>> +	 * Driver attrs are tuples of {key, [print-type], value}.
>> +	 * The key must be a string.  If print-type is present, it
>> +	 * defines an alternate printf format type vs the native format
>> +	 * for the attribute.  And the value can be any available
>> +	 * driver type.
>> +	 */
>> +	mnl_attr_for_each_nested(tb_entry, tb) {
>> +
>> +		if (cc > MAX_LINE_LENGTH) {
>> +			if (rd->pretty_output)
>> +				newline_indent(rd);
>> +			cc = 0;
>> +		}
>> +		if (rd_attr_check(tb_entry, &type) != MNL_CB_OK)
>> +			return;
>> +		if (!key) {
>> +			if (type != MNL_TYPE_NUL_STRING)
>> +				return;
>> +			key = tb_entry;
>> +		} else if (type == MNL_TYPE_U8) {
>> +			print_type = mnl_attr_get_u8(tb_entry);
>> +		} else {
>> +			val = tb_entry;
>> +			cc += print_driver_entry(rd, key, val, print_type);
> I stopped to read here, because of two problems:
> 1. print_driver_entry can return negative number, so unclear to me what
> will be the final result of "cc += ..".
> 2. The netlink design is to ignore unknown attributes and not return
> error. It allows to use new kernels with old applications.

Yes, this is a bug.  See below the check for 'cc < 0':

>> +			if (cc < 0)
>> +				return;

It's incorrect.  The return from print_driver_entry() should be checked
for error to allow ignoring unknown attrs, and then if no error, cc gets
incremented.  I'll fix this up.


Thanks for reviewing,

Steve.

>> +			print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC;
>> +			key = NULL;
>> +		}
>> +	}
>> +	return;
>> +}
>> --
>> 1.8.3.1
>>

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote:
> 
> On 5/13/2018 8:24 AM, Leon Romanovsky wrote:
> > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > This enhancement allows printing rdma device-specific state, if provided
> > > by the kernel.  This is done in a generic manner, so rdma tool doesn't
> > 
> > Double space between "." and "This".
> > 
> > > need to know about the details of every type of rdma device.
> > > 
> > > Driver attributes for a rdma resource are in the form of <key,
> > > [print_type], value> tuples, where the key is a string and the value can
> > > be any supported driver attribute.  The print_type attribute, if present,
> > 
> > ditto
> 
> I'll fix these.

Fix it if you want, but don't do it because Leon told you to.  A double
space after period is perfectly acceptable.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote:
> On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote:
> >
> > On 5/13/2018 8:24 AM, Leon Romanovsky wrote:
> > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > > This enhancement allows printing rdma device-specific state, if provided
> > > > by the kernel.  This is done in a generic manner, so rdma tool doesn't
> > >
> > > Double space between "." and "This".
> > >
> > > > need to know about the details of every type of rdma device.
> > > >
> > > > Driver attributes for a rdma resource are in the form of <key,
> > > > [print_type], value> tuples, where the key is a string and the value can
> > > > be any supported driver attribute.  The print_type attribute, if present,
> > >
> > > ditto
> >
> > I'll fix these.
>
> Fix it if you want, but don't do it because Leon told you to.  A double
> space after period is perfectly acceptable.

It is very controversial thing [1],

"Most style guides indicate that single sentence spacing is proper for
final or published work today, and most publishers require manuscripts
to be submitted as they will appear in publication—single
sentence spaced."

[1] https://en.wikipedia.org/wiki/Sentence_spacing

>
> --
> Doug Ledford <dledford@redhat.com>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

RE: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

> 
> On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote:
> > On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote:
> > >
> > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote:
> > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > > > This enhancement allows printing rdma device-specific state, if
> provided
> > > > > by the kernel.  This is done in a generic manner, so rdma tool doesn't
> > > >
> > > > Double space between "." and "This".
> > > >
> > > > > need to know about the details of every type of rdma device.
> > > > >
> > > > > Driver attributes for a rdma resource are in the form of <key,
> > > > > [print_type], value> tuples, where the key is a string and the value can
> > > > > be any supported driver attribute.  The print_type attribute, if
> present,
> > > >
> > > > ditto
> > >
> > > I'll fix these.
> >
> > Fix it if you want, but don't do it because Leon told you to.  A double
> > space after period is perfectly acceptable.
> 
> It is very controversial thing [1],
> 
> "Most style guides indicate that single sentence spacing is proper for
> final or published work today, and most publishers require manuscripts
> to be submitted as they will appear in publication—single
> sentence spaced."
> 
> [1] https://en.wikipedia.org/wiki/Sentence_spacing

We're not writing  a manuscript. 😉  Regardless, I made the changes and they are in v2 of the patch series, which I think is probably ready to merge.

Steve.

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Tue, 2018-05-15 at 19:59 +0300, Leon Romanovsky wrote:
> On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote:
> > On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote:
> > > 
> > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote:
> > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > > > This enhancement allows printing rdma device-specific state, if provided
> > > > > by the kernel.  This is done in a generic manner, so rdma tool doesn't
> > > > 
> > > > Double space between "." and "This".
> > > > 
> > > > > need to know about the details of every type of rdma device.
> > > > > 
> > > > > Driver attributes for a rdma resource are in the form of <key,
> > > > > [print_type], value> tuples, where the key is a string and the value can
> > > > > be any supported driver attribute.  The print_type attribute, if present,
> > > > 
> > > > ditto
> > > 
> > > I'll fix these.
> > 
> > Fix it if you want, but don't do it because Leon told you to.  A double
> > space after period is perfectly acceptable.
> 
> It is very controversial thing [1],
> 
> "Most style guides indicate that single sentence spacing is proper for
> final or published work today, and most publishers require manuscripts
> to be submitted as they will appear in publication—single
> sentence spaced."
> 
> [1] https://en.wikipedia.org/wiki/Sentence_spacing

Yes...and the justification is that proportional font systems resolve
the issue of delineating sentences without the need for a second space
(they actually don't because they are improperly implemented, but that's
another issue).  But we don't work on proportional font systems, we work
with git, on command lines, with fixed spacing fonts, all the things
that indicate double spaces are actually preferred even by those people
that are proponents of single spacing.  And on top of that, current
research suggests maybe the single spacers are wrong after all :-o

https://www.theatlantic.com/science/archive/2018/05/two-spaces-after-a-p
eriod/559304/

Anyway, my original point stands.  It's controversial, as you said, so
Steve should do what Steve feels is best and not worry about anyone
else' opinion on the matter.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Jason Gunthorpe]

On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> This enhancement allows printing rdma device-specific state, if provided
> by the kernel.  This is done in a generic manner, so rdma tool doesn't
> need to know about the details of every type of rdma device.
> 
> Driver attributes for a rdma resource are in the form of <key,
> [print_type], value> tuples, where the key is a string and the value can
> be any supported driver attribute.  The print_type attribute, if present,
> provides a print format to use vs the standard print format for the type.
> For example, the default print type for a PROVIDER_S32 value is "%d ",
> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.
> 
> Driver resources are only printed when the -dd flag is present.
> If -p is present, then the output is formatted to not exceed 80 columns,
> otherwise it is printed as a single row to be grep/awk friendly.
> 
> Example output:
> 
> # rdma resource show qp lqpn 1028 -dd -p
> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0

Hey some of these look like kernel pointers.. That is a no-no.. What
is up there?

The wr_id often contains a pointer, right? So we cannot just pass it
to user space..

Jason

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
>> This enhancement allows printing rdma device-specific state, if provided
>> by the kernel.  This is done in a generic manner, so rdma tool doesn't
>> need to know about the details of every type of rdma device.
>>
>> Driver attributes for a rdma resource are in the form of <key,
>> [print_type], value> tuples, where the key is a string and the value can
>> be any supported driver attribute.  The print_type attribute, if present,
>> provides a print format to use vs the standard print format for the type.
>> For example, the default print type for a PROVIDER_S32 value is "%d ",
>> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.
>>
>> Driver resources are only printed when the -dd flag is present.
>> If -p is present, then the output is formatted to not exceed 80 columns,
>> otherwise it is printed as a single row to be grep/awk friendly.
>>
>> Example output:
>>
>> # rdma resource show qp lqpn 1028 -dd -p
>> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
>>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
>>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
>>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0
> Hey some of these look like kernel pointers.. That is a no-no.. What
> is up there?

Nothing is defined as a kernel pointer.  But wr_id is often a pointer to
the kernel rdma application's context...

> The wr_id often contains a pointer, right? So we cannot just pass it
> to user space..

Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
these attrs can be only be sent up by the kernel if the capabilities
allow.  But previous review comments of the kernel series, which is now
merged, forced me to remove passing the capabilities information to the
driver resource fill functions. 

So what's the right way to do this?

Steve.

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote:
>
>
> On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> >> This enhancement allows printing rdma device-specific state, if provided
> >> by the kernel.  This is done in a generic manner, so rdma tool doesn't
> >> need to know about the details of every type of rdma device.
> >>
> >> Driver attributes for a rdma resource are in the form of <key,
> >> [print_type], value> tuples, where the key is a string and the value can
> >> be any supported driver attribute.  The print_type attribute, if present,
> >> provides a print format to use vs the standard print format for the type.
> >> For example, the default print type for a PROVIDER_S32 value is "%d ",
> >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple.
> >>
> >> Driver resources are only printed when the -dd flag is present.
> >> If -p is present, then the output is formatted to not exceed 80 columns,
> >> otherwise it is printed as a single row to be grep/awk friendly.
> >>
> >> Example output:
> >>
> >> # rdma resource show qp lqpn 1028 -dd -p
> >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma]
> >>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0
> >>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00
> >>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0
> > Hey some of these look like kernel pointers.. That is a no-no.. What
> > is up there?
>
> Nothing is defined as a kernel pointer.  But wr_id is often a pointer to
> the kernel rdma application's context...
>
> > The wr_id often contains a pointer, right? So we cannot just pass it
> > to user space..
>
> Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
> these attrs can be only be sent up by the kernel if the capabilities
> allow.  But previous review comments of the kernel series, which is now
> merged, forced me to remove passing the capabilities information to the
> driver resource fill functions. 
>
> So what's the right way to do this?

The reviewer asked do not pass to drivers whole CAP_.. bits, because
they anyway don't need such granularity.

>
> Steve.
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

RE: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

> On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote:
> >
> >
> > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > >> This enhancement allows printing rdma device-specific state, if
provided
> > >> by the kernel.  This is done in a generic manner, so rdma tool
doesn't
> > >> need to know about the details of every type of rdma device.
> > >>
> > >> Driver attributes for a rdma resource are in the form of <key,
> > >> [print_type], value> tuples, where the key is a string and the value
can
> > >> be any supported driver attribute.  The print_type attribute, if
present,
> > >> provides a print format to use vs the standard print format for the
type.
> > >> For example, the default print type for a PROVIDER_S32 value is "%d
",
> > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe
tuple.
> > >>
> > >> Driver resources are only printed when the -dd flag is present.
> > >> If -p is present, then the output is formatted to not exceed 80
columns,
> > >> otherwise it is printed as a single row to be grep/awk friendly.
> > >>
> > >> Example output:
> > >>
> > >> # rdma resource show qp lqpn 1028 -dd -p
> > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0
> path-mig-state MIGRATED pid 0 comm [nvme_rdma]
> > >>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106
> flush_cidx 85 in_use 0
> > >>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41
wq_pidx
> 171 msn 44 rqt_hwaddr 0x2a8a5d00
> > >>     rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408
idx
> 40 wr_id 0xffff881057c033f0
> > > Hey some of these look like kernel pointers.. That is a no-no.. What
> > > is up there?
> >
> > Nothing is defined as a kernel pointer.  But wr_id is often a pointer to
> > the kernel rdma application's context...
> >
> > > The wr_id often contains a pointer, right? So we cannot just pass it
> > > to user space..
> >
> > Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
> > these attrs can be only be sent up by the kernel if the capabilities
> > allow.  But previous review comments of the kernel series, which is now
> > merged, forced me to remove passing the capabilities information to the
> > driver resource fill functions.
> >
> > So what's the right way to do this?
> 
> The reviewer asked do not pass to drivers whole CAP_.. bits, because
> they anyway don't need such granularity.
> 

Ok thanks.

RE: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

 
> > On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote:
> > >
> > >
> > > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > >> This enhancement allows printing rdma device-specific state, if
> provided
> > > >> by the kernel.  This is done in a generic manner, so rdma tool
doesn't
> > > >> need to know about the details of every type of rdma device.
> > > >>
> > > >> Driver attributes for a rdma resource are in the form of <key,
> > > >> [print_type], value> tuples, where the key is a string and the
value can
> > > >> be any supported driver attribute.  The print_type attribute, if
present,
> > > >> provides a print format to use vs the standard print format for the
> type.
> > > >> For example, the default print type for a PROVIDER_S32 value is "%d
",
> > > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe
> tuple.
> > > >>
> > > >> Driver resources are only printed when the -dd flag is present.
> > > >> If -p is present, then the output is formatted to not exceed 80
> columns,
> > > >> otherwise it is printed as a single row to be grep/awk friendly.
> > > >>
> > > >> Example output:
> > > >>
> > > >> # rdma resource show qp lqpn 1028 -dd -p
> > > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0
> > path-mig-state MIGRATED pid 0 comm [nvme_rdma]
> > > >>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106
> > flush_cidx 85 in_use 0
> > > >>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41
wq_pidx
> > 171 msn 44 rqt_hwaddr 0x2a8a5d00
> > > >>     rqt_size 256 in_use 128 size 130 idx 43 wr_id
0xffff881057c03408 idx
> > 40 wr_id 0xffff881057c033f0
> > > > Hey some of these look like kernel pointers.. That is a no-no.. What
> > > > is up there?
> > >
> > > Nothing is defined as a kernel pointer.  But wr_id is often a pointer
to
> > > the kernel rdma application's context...
> > >
> > > > The wr_id often contains a pointer, right? So we cannot just pass it
> > > > to user space..
> > >
> > > Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
> > > these attrs can be only be sent up by the kernel if the capabilities
> > > allow.  But previous review comments of the kernel series, which is
now
> > > merged, forced me to remove passing the capabilities information to
the
> > > driver resource fill functions.
> > >
> > > So what's the right way to do this?
> >
> > The reviewer asked do not pass to drivers whole CAP_.. bits, because
> > they anyway don't need such granularity.
> >
> 
> Ok thanks.

How's this?

diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
index 6379685..2cf9c5c 100644
--- a/include/rdma/restrack.h
+++ b/include/rdma/restrack.h
@@ -66,7 +66,8 @@ struct rdma_restrack_root {
         * Allows rdma drivers to add their own restrack attributes.
         */
        int (*fill_res_entry)(struct sk_buff *msg,
-                             struct rdma_restrack_entry *entry);
+                             struct rdma_restrack_entry *entry,
+                             bool net_admin_capable);
 };

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Jason Gunthorpe]

On Tue, May 15, 2018 at 08:18:51AM -0500, Steve Wise wrote:
>  
> > > On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote:
> > > >
> > > >
> > > > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> > > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > > >> This enhancement allows printing rdma device-specific state, if
> > provided
> > > > >> by the kernel.  This is done in a generic manner, so rdma tool
> doesn't
> > > > >> need to know about the details of every type of rdma device.
> > > > >>
> > > > >> Driver attributes for a rdma resource are in the form of <key,
> > > > >> [print_type], value> tuples, where the key is a string and the
> value can
> > > > >> be any supported driver attribute.  The print_type attribute, if
> present,
> > > > >> provides a print format to use vs the standard print format for the
> > type.
> > > > >> For example, the default print type for a PROVIDER_S32 value is "%d
> ",
> > > > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe
> > tuple.
> > > > >>
> > > > >> Driver resources are only printed when the -dd flag is present.
> > > > >> If -p is present, then the output is formatted to not exceed 80
> > columns,
> > > > >> otherwise it is printed as a single row to be grep/awk friendly.
> > > > >>
> > > > >> Example output:
> > > > >>
> > > > >> # rdma resource show qp lqpn 1028 -dd -p
> > > > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0
> > > path-mig-state MIGRATED pid 0 comm [nvme_rdma]
> > > > >>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106
> > > flush_cidx 85 in_use 0
> > > > >>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41
> wq_pidx
> > > 171 msn 44 rqt_hwaddr 0x2a8a5d00
> > > > >>     rqt_size 256 in_use 128 size 130 idx 43 wr_id
> 0xffff881057c03408 idx
> > > 40 wr_id 0xffff881057c033f0
> > > > > Hey some of these look like kernel pointers.. That is a no-no.. What
> > > > > is up there?
> > > >
> > > > Nothing is defined as a kernel pointer.  But wr_id is often a pointer
> to
> > > > the kernel rdma application's context...
> > > >
> > > > > The wr_id often contains a pointer, right? So we cannot just pass it
> > > > > to user space..
> > > >
> > > > Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
> > > > these attrs can be only be sent up by the kernel if the capabilities
> > > > allow.  But previous review comments of the kernel series, which is
> now
> > > > merged, forced me to remove passing the capabilities information to
> the
> > > > driver resource fill functions.
> > > >
> > > > So what's the right way to do this?
> > >
> > > The reviewer asked do not pass to drivers whole CAP_.. bits, because
> > > they anyway don't need such granularity.
> > >
> > 
> > Ok thanks.
> 
> How's this?
> 
> diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> index 6379685..2cf9c5c 100644
> +++ b/include/rdma/restrack.h
> @@ -66,7 +66,8 @@ struct rdma_restrack_root {
>          * Allows rdma drivers to add their own restrack attributes.
>          */
>         int (*fill_res_entry)(struct sk_buff *msg,
> -                             struct rdma_restrack_entry *entry);
> +                             struct rdma_restrack_entry *entry,
> +                             bool net_admin_capable);
>  };

cap net admin is not high enough privledge to see unhashed kernel
pointers. CAP_RAW_IO? Or follow what printk does?

Honestly, I don't really know, this hashed pointer stuff is new..

Jason

RE: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> On Tue, May 15, 2018 at 08:18:51AM -0500, Steve Wise wrote:
> >
> > > > On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote:
> > > > >
> > > > >
> > > > > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote:
> > > > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote:
> > > > > >> This enhancement allows printing rdma device-specific state, if
> > > provided
> > > > > >> by the kernel.  This is done in a generic manner, so rdma tool
> > doesn't
> > > > > >> need to know about the details of every type of rdma device.
> > > > > >>
> > > > > >> Driver attributes for a rdma resource are in the form of <key,
> > > > > >> [print_type], value> tuples, where the key is a string and the
> > value can
> > > > > >> be any supported driver attribute.  The print_type attribute, if
> > present,
> > > > > >> provides a print format to use vs the standard print format for the
> > > type.
> > > > > >> For example, the default print type for a PROVIDER_S32 value is
> "%d
> > ",
> > > > > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe
> > > tuple.
> > > > > >>
> > > > > >> Driver resources are only printed when the -dd flag is present.
> > > > > >> If -p is present, then the output is formatted to not exceed 80
> > > columns,
> > > > > >> otherwise it is printed as a single row to be grep/awk friendly.
> > > > > >>
> > > > > >> Example output:
> > > > > >>
> > > > > >> # rdma resource show qp lqpn 1028 -dd -p
> > > > > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0
> > > > path-mig-state MIGRATED pid 0 comm [nvme_rdma]
> > > > > >>     sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106
> > > > flush_cidx 85 in_use 0
> > > > > >>     size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41
> > wq_pidx
> > > > 171 msn 44 rqt_hwaddr 0x2a8a5d00
> > > > > >>     rqt_size 256 in_use 128 size 130 idx 43 wr_id
> > 0xffff881057c03408 idx
> > > > 40 wr_id 0xffff881057c033f0
> > > > > > Hey some of these look like kernel pointers.. That is a no-no.. What
> > > > > > is up there?
> > > > >
> > > > > Nothing is defined as a kernel pointer.  But wr_id is often a pointer
> > to
> > > > > the kernel rdma application's context...
> > > > >
> > > > > > The wr_id often contains a pointer, right? So we cannot just pass it
> > > > > > to user space..
> > > > >
> > > > > Hmm.  It is useful for debugging kernel rdma applications.  Perhaps
> > > > > these attrs can be only be sent up by the kernel if the capabilities
> > > > > allow.  But previous review comments of the kernel series, which is
> > now
> > > > > merged, forced me to remove passing the capabilities information to
> > the
> > > > > driver resource fill functions.
> > > > >
> > > > > So what's the right way to do this?
> > > >
> > > > The reviewer asked do not pass to drivers whole CAP_.. bits, because
> > > > they anyway don't need such granularity.
> > > >
> > >
> > > Ok thanks.
> >
> > How's this?
> >
> > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > index 6379685..2cf9c5c 100644
> > +++ b/include/rdma/restrack.h
> > @@ -66,7 +66,8 @@ struct rdma_restrack_root {
> >          * Allows rdma drivers to add their own restrack attributes.
> >          */
> >         int (*fill_res_entry)(struct sk_buff *msg,
> > -                             struct rdma_restrack_entry *entry);
> > +                             struct rdma_restrack_entry *entry,
> > +                             bool net_admin_capable);
> >  };
> 
> cap net admin is not high enough privledge to see unhashed kernel
> pointers. CAP_RAW_IO? Or follow what printk does?
> 

Do you mean CAP_NET_RAW?  Here's the comments for it:

/* Allow use of RAW sockets */
/* Allow use of PACKET sockets */
/* Allow binding to any address for transparent proxying (also via NET_ADMIN) */


Func restricted_pointer() from lib/vsprintf.c uses CAP_SYSLOG.  The comment for CAP_SYSLOG:

/* Allow configuring the kernel's syslog (printk behaviour) */

Func kallsyms_show_value() also uses CAP_SYSLOG.  And there is a non-exported global kptr_restrict that they use apparently to allow overriding all this for profiling.

Here is NET_ADMIN's comments:

/* Allow interface configuration */
/* Allow administration of IP firewall, masquerading and accounting */
/* Allow setting debug option on sockets */
/* Allow modification of routing tables */
/* Allow setting arbitrary process / process group ownership on
   sockets */
/* Allow binding to any address for transparent proxying (also via NET_RAW) */
/* Allow setting TOS (type of service) */
/* Allow setting promiscuous mode */
/* Allow clearing driver statistics */
/* Allow multicasting */
/* Allow read/write of device-specific registers */
/* Allow activation of ATM control sockets */

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Jason Gunthorpe]

On Tue, May 15, 2018 at 09:31:27AM -0500, Steve Wise wrote:
> > cap net admin is not high enough privledge to see unhashed kernel
> > pointers. CAP_RAW_IO? Or follow what printk does?
> > 
> 
> Do you mean CAP_NET_RAW?  Here's the comments for it:

Nope..

> Func restricted_pointer() from lib/vsprintf.c uses CAP_SYSLOG.  The comment for CAP_SYSLOG:

Yikes, yes, that is probably the required logic here, including the
kptr_restrict = 0 thing

Jason

RE: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Steve Wise]

> On Tue, May 15, 2018 at 09:31:27AM -0500, Steve Wise wrote:
> > > cap net admin is not high enough privledge to see unhashed kernel
> > > pointers. CAP_RAW_IO? Or follow what printk does?
> > >
> >
> > Do you mean CAP_NET_RAW?  Here's the comments for it:
> 
> Nope..
> 
> > Func restricted_pointer() from lib/vsprintf.c uses CAP_SYSLOG.  The
> comment for CAP_SYSLOG:
> 
> Yikes, yes, that is probably the required logic here, including the
> kptr_restrict = 0 thing
> 

Let's defer the ktpr_restrict issue for now; I want to finish the initial
work this cycle, and adding that will likely take too much time.   I'll use
CAP_SYSLOG and add a FIXME comment.  Ok? 

Steve.

Re: [PATCH v1 iproute2-next 2/3] rdma: print driver resource attributes

[Jason Gunthorpe]

On Tue, May 15, 2018 at 10:02:08AM -0500, Steve Wise wrote:
> > On Tue, May 15, 2018 at 09:31:27AM -0500, Steve Wise wrote:
> > > > cap net admin is not high enough privledge to see unhashed kernel
> > > > pointers. CAP_RAW_IO? Or follow what printk does?
> > > >
> > >
> > > Do you mean CAP_NET_RAW?  Here's the comments for it:
> > 
> > Nope..
> > 
> > > Func restricted_pointer() from lib/vsprintf.c uses CAP_SYSLOG.  The
> > comment for CAP_SYSLOG:
> > 
> > Yikes, yes, that is probably the required logic here, including the
> > kptr_restrict = 0 thing
> > 
> 
> Let's defer the ktpr_restrict issue for now; I want to finish the initial
> work this cycle, and adding that will likely take too much time.   I'll use
> CAP_SYSLOG and add a FIXME comment.  Ok? 

Lets just drop wr_id instead...

Jason

[PATCH v1 iproute2-next 3/3] rdma: update man pages

[Steve Wise]

Update the man pages for the resource attributes as well
as the driver-specific attributes.

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 man/man8/rdma-resource.8 | 29 ++++++++++++++++++++++++++---
 man/man8/rdma.8          |  2 +-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/man/man8/rdma-resource.8 b/man/man8/rdma-resource.8
index ff5d25d..40b073d 100644
--- a/man/man8/rdma-resource.8
+++ b/man/man8/rdma-resource.8
@@ -7,13 +7,16 @@ rdma-resource \- rdma resource configuration
 .in +8
 .ti -8
 .B rdma
-.RI "[ " OPTIONS " ]"
-.B resource
-.RI  " { " COMMAND " | "
+.RI "[ " OPTIONS " ] " RESOURCE " { " COMMAND " | "
 .BR help " }"
 .sp
 
 .ti -8
+.IR RESOURCE " := { "
+.BR cm_id " | " cq " | " mr " | " pd " | " qp " }"
+.sp
+
+.ti -8
 .IR OPTIONS " := { "
 \fB\-j\fR[\fIson\fR] |
 \fB\-d\fR[\fIetails\fR] }
@@ -70,11 +73,31 @@ rdma res show qp link mlx5_4/- -d
 Detailed view.
 .RE
 .PP
+rdma res show qp link mlx5_4/- -dd
+.RS 4
+Detailed view including driver-specific details.
+.RE
+.PP
 rdma res show qp link mlx5_4/1 lqpn 0-6
 .RS 4
 Limit to specific Local QPNs.
 .RE
 .PP
+rdma resource show cm_id dst-port 7174
+.RS 4
+Show CM_IDs with destination ip port of 7174.
+.RE
+.PP
+rdma resource show cm_id src-addr 172.16.0.100
+.RS 4
+Show CM_IDs bound to local ip address 172.16.0.100
+.RE
+.PP
+rdma resource show cq pid 30489
+.RS 4
+Show CQs belonging to pid 30489
+.RE
+.PP
 
 .SH SEE ALSO
 .BR rdma (8),
diff --git a/man/man8/rdma.8 b/man/man8/rdma.8
index 6f88f37..12aa149 100644
--- a/man/man8/rdma.8
+++ b/man/man8/rdma.8
@@ -49,7 +49,7 @@ If there were any errors during execution of the commands, the application retur
 
 .TP
 .BR "\-d" , " --details"
-Output detailed information.
+Output detailed information.  Adding a second \-d includes driver-specific details.
 
 .TP
 .BR "\-p" , " --pretty"
-- 
1.8.3.1