[PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

This function sends the constructed netlink message and then
receives the response, displaying any error text.

Change 'rdma dev set' to use it.

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 rdma/dev.c   |  2 +-
 rdma/rdma.h  |  1 +
 rdma/utils.c | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/rdma/dev.c b/rdma/dev.c
index 60ff4b31e320..d2949c378f08 100644
--- a/rdma/dev.c
+++ b/rdma/dev.c
@@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
 	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
 	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
 
-	return rd_send_msg(rd);
+	return rd_sendrecv_msg(rd, seq);
 }
 
 static int dev_one_set(struct rd *rd)
diff --git a/rdma/rdma.h b/rdma/rdma.h
index 547bb5749a39..20be2f12c4f8 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
  */
 int rd_send_msg(struct rd *rd);
 int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
+int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
diff --git a/rdma/utils.c b/rdma/utils.c
index 069d44fece10..a6f2826c9605 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
 	return ret;
 }
 
+static int null_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
+{
+	int ret;
+
+	ret = rd_send_msg(rd);
+	if (ret) {
+		perror(NULL);
+		goto out;
+	}
+	ret = rd_recv_msg(rd, null_cb, rd, seq);
+	if (ret)
+		perror(NULL);
+out:
+	return ret;
+}
+
 static struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name)
 {
 	struct dev_map *dev_map;
-- 
1.8.3.1

[PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Steve Wise]

Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
RDMA_NLDEV_CMD_DELLINK API.

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

diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
index 04c80cebef49..23a90ad52485 100644
--- a/rdma/include/uapi/rdma/rdma_netlink.h
+++ b/rdma/include/uapi/rdma/rdma_netlink.h
@@ -5,8 +5,7 @@
 #include <linux/types.h>
 
 enum {
-	RDMA_NL_RDMA_CM = 1,
-	RDMA_NL_IWCM,
+	RDMA_NL_IWCM = 2,
 	RDMA_NL_RSVD,
 	RDMA_NL_LS,	/* RDMA Local Services */
 	RDMA_NL_NLDEV,	/* RDMA device interface */
@@ -14,8 +13,7 @@ enum {
 };
 
 enum {
-	RDMA_NL_GROUP_CM = 1,
-	RDMA_NL_GROUP_IWPM,
+	RDMA_NL_GROUP_IWPM = 2,
 	RDMA_NL_GROUP_LS,
 	RDMA_NL_NUM_GROUPS
 };
@@ -24,15 +22,17 @@ enum {
 #define RDMA_NL_GET_OP(type) (type & ((1 << 10) - 1))
 #define RDMA_NL_GET_TYPE(client, op) ((client << 10) + op)
 
-enum {
-	RDMA_NL_RDMA_CM_ID_STATS = 0,
-	RDMA_NL_RDMA_CM_NUM_OPS
-};
+/* The minimum version that the iwpm kernel supports */
+#define IWPM_UABI_VERSION_MIN	3
+
+/* The latest version that the iwpm kernel supports */
+#define IWPM_UABI_VERSION	4
 
+/* iwarp port mapper message flags */
 enum {
-	RDMA_NL_RDMA_CM_ATTR_SRC_ADDR = 1,
-	RDMA_NL_RDMA_CM_ATTR_DST_ADDR,
-	RDMA_NL_RDMA_CM_NUM_ATTR,
+
+	/* Do not map the port for this IWPM request */
+	IWPM_FLAGS_NO_PORT_MAP = (1 << 0),
 };
 
 /* iwarp port mapper op-codes */
@@ -45,6 +45,7 @@ enum {
 	RDMA_NL_IWPM_HANDLE_ERR,
 	RDMA_NL_IWPM_MAPINFO,
 	RDMA_NL_IWPM_MAPINFO_NUM,
+	RDMA_NL_IWPM_HELLO,
 	RDMA_NL_IWPM_NUM_OPS
 };
 
@@ -83,20 +84,38 @@ enum {
 	IWPM_NLA_MANAGE_MAPPING_UNSPEC = 0,
 	IWPM_NLA_MANAGE_MAPPING_SEQ,
 	IWPM_NLA_MANAGE_ADDR,
-	IWPM_NLA_MANAGE_MAPPED_LOC_ADDR,
+	IWPM_NLA_MANAGE_FLAGS,
+	IWPM_NLA_MANAGE_MAPPING_MAX
+};
+
+enum {
+	IWPM_NLA_RMANAGE_MAPPING_UNSPEC = 0,
+	IWPM_NLA_RMANAGE_MAPPING_SEQ,
+	IWPM_NLA_RMANAGE_ADDR,
+	IWPM_NLA_RMANAGE_MAPPED_LOC_ADDR,
+	/* The following maintains bisectability of rdma-core */
+	IWPM_NLA_MANAGE_MAPPED_LOC_ADDR = IWPM_NLA_RMANAGE_MAPPED_LOC_ADDR,
 	IWPM_NLA_RMANAGE_MAPPING_ERR,
 	IWPM_NLA_RMANAGE_MAPPING_MAX
 };
 
-#define IWPM_NLA_MANAGE_MAPPING_MAX 3
-#define IWPM_NLA_QUERY_MAPPING_MAX  4
 #define IWPM_NLA_MAPINFO_SEND_MAX   3
+#define IWPM_NLA_REMOVE_MAPPING_MAX 3
 
 enum {
 	IWPM_NLA_QUERY_MAPPING_UNSPEC = 0,
 	IWPM_NLA_QUERY_MAPPING_SEQ,
 	IWPM_NLA_QUERY_LOCAL_ADDR,
 	IWPM_NLA_QUERY_REMOTE_ADDR,
+	IWPM_NLA_QUERY_FLAGS,
+	IWPM_NLA_QUERY_MAPPING_MAX,
+};
+
+enum {
+	IWPM_NLA_RQUERY_MAPPING_UNSPEC = 0,
+	IWPM_NLA_RQUERY_MAPPING_SEQ,
+	IWPM_NLA_RQUERY_LOCAL_ADDR,
+	IWPM_NLA_RQUERY_REMOTE_ADDR,
 	IWPM_NLA_RQUERY_MAPPED_LOC_ADDR,
 	IWPM_NLA_RQUERY_MAPPED_REM_ADDR,
 	IWPM_NLA_RQUERY_MAPPING_ERR,
@@ -114,6 +133,7 @@ enum {
 	IWPM_NLA_MAPINFO_UNSPEC = 0,
 	IWPM_NLA_MAPINFO_LOCAL_ADDR,
 	IWPM_NLA_MAPINFO_MAPPED_ADDR,
+	IWPM_NLA_MAPINFO_FLAGS,
 	IWPM_NLA_MAPINFO_MAX
 };
 
@@ -132,6 +152,12 @@ enum {
 	IWPM_NLA_ERR_MAX
 };
 
+enum {
+	IWPM_NLA_HELLO_UNSPEC = 0,
+	IWPM_NLA_HELLO_ABI_VERSION,
+	IWPM_NLA_HELLO_MAX
+};
+
 /*
  * Local service operations:
  *   RESOLVE - The client requests the local service to resolve a path.
@@ -229,9 +255,11 @@ enum rdma_nldev_command {
 	RDMA_NLDEV_CMD_GET, /* can dump */
 	RDMA_NLDEV_CMD_SET,
 
-	/* 3 - 4 are free to use */
+	RDMA_NLDEV_CMD_NEWLINK,
 
-	RDMA_NLDEV_CMD_PORT_GET = 5, /* can dump */
+	RDMA_NLDEV_CMD_DELLINK,
+
+	RDMA_NLDEV_CMD_PORT_GET, /* can dump */
 
 	/* 6 - 8 are free to use */
 
@@ -431,6 +459,20 @@ enum rdma_nldev_attr {
 	RDMA_NLDEV_ATTR_DRIVER_U64,		/* u64 */
 
 	/*
+	 * Indexes to get/set secific entry,
+	 * for QP use RDMA_NLDEV_ATTR_RES_LQPN
+	 */
+	RDMA_NLDEV_ATTR_RES_PDN,               /* u32 */
+	RDMA_NLDEV_ATTR_RES_CQN,               /* u32 */
+	RDMA_NLDEV_ATTR_RES_MRN,               /* u32 */
+	RDMA_NLDEV_ATTR_RES_CM_IDN,            /* u32 */
+	RDMA_NLDEV_ATTR_RES_CTXN,	       /* u32 */
+	/*
+	 * Identifies the rdma driver. eg: "rxe" or "siw"
+	 */
+	RDMA_NLDEV_ATTR_LINK_TYPE,		/* string */
+
+	/*
 	 * Always the end
 	 */
 	RDMA_NLDEV_ATTR_MAX
-- 
1.8.3.1

[PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Steve Wise]

Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
device to a netdev interface.

EG:

rdma link add rxe_eth0 type rxe netdev eth0
rdma link delete rxe_eth0

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 rdma/link.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |  1 +
 rdma/utils.c |  2 +-
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/rdma/link.c b/rdma/link.c
index c064be627be2..afaf19663728 100644
--- a/rdma/link.c
+++ b/rdma/link.c
@@ -14,6 +14,9 @@
 static int link_help(struct rd *rd)
 {
 	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
+	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
+	       rd->filename);
+	pr_out("Usage: %s link delete NAME\n", rd->filename);
 	return 0;
 }
 
@@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
 	return rd_exec_link(rd, link_one_show, true);
 }
 
+static int link_add(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	char *dev = NULL;
+	uint32_t seq;
+
+	if (rd_no_arg(rd)) {
+		pr_err("No link name was supplied\n");
+		return -EINVAL;
+	}
+	name = rd_argv(rd);
+	rd_arg_inc(rd);
+	while (!rd_no_arg(rd)) {
+		if (rd_argv_match(rd, "type")) {
+			rd_arg_inc(rd);
+			type = rd_argv(rd);
+		} else if (rd_argv_match(rd, "netdev")) {
+			rd_arg_inc(rd);
+			dev = rd_argv(rd);
+		} else {
+			pr_err("Invalid parameter %s\n", rd_argv(rd));
+			return -EINVAL;
+		}
+		rd_arg_inc(rd);
+	}
+	if (!type) {
+		pr_err("No type was supplied\n");
+		return -EINVAL;
+	}
+	if (!dev) {
+		pr_err("No net device was supplied\n");
+		return -EINVAL;
+	}
+
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_NEWLINK, &seq,
+		       (NLM_F_REQUEST | NLM_F_ACK));
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_NDEV_NAME, dev);
+	return rd_sendrecv_msg(rd, seq);
+}
+
+static int _link_del(struct rd *rd)
+{
+	uint32_t seq;
+
+	if (!rd_no_arg(rd)) {
+		pr_err("Unknown parameter %s\n", rd_argv(rd));
+		return -EINVAL;
+	}
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
+		       (NLM_F_REQUEST | NLM_F_ACK));
+	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
+	return rd_sendrecv_msg(rd, seq);
+}
+
+static int link_del(struct rd *rd)
+{
+	return rd_exec_require_dev(rd, _link_del);
+}
+
 int cmd_link(struct rd *rd)
 {
 	const struct rd_cmd cmds[] = {
 		{ NULL,		link_show },
+		{ "add",	link_add },
+		{ "delete",	link_del },
 		{ "show",	link_show },
 		{ "list",	link_show },
 		{ "help",	link_help },
diff --git a/rdma/rdma.h b/rdma/rdma.h
index 20be2f12c4f8..af4597f45640 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -79,6 +79,7 @@ struct rd_cmd {
  */
 bool rd_no_arg(struct rd *rd);
 void rd_arg_inc(struct rd *rd);
+bool rd_argv_match(struct rd *rd, const char *pattern);
 
 char *rd_argv(struct rd *rd);
 
diff --git a/rdma/utils.c b/rdma/utils.c
index a6f2826c9605..85a954167511 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -32,7 +32,7 @@ int strcmpx(const char *str1, const char *str2)
 	return strncmp(str1, str2, strlen(str1));
 }
 
-static bool rd_argv_match(struct rd *rd, const char *pattern)
+bool rd_argv_match(struct rd *rd, const char *pattern)
 {
 	if (!rd_argc(rd))
 		return false;
-- 
1.8.3.1

[PATCH v1 iproute2-next 4/4] rdma: man page update for link add/delete

[Steve Wise]

Update the 'rdma link' man page with 'link add/delete' info.

Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 man/man8/rdma-link.8 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/man/man8/rdma-link.8 b/man/man8/rdma-link.8
index bddf34746e8b..b3b40de75852 100644
--- a/man/man8/rdma-link.8
+++ b/man/man8/rdma-link.8
@@ -23,6 +23,18 @@ rdma-link \- rdma link configuration
 .RI "[ " DEV/PORT_INDEX " ]"
 
 .ti -8
+.B rdma link add
+.BR NAME
+.BR type
+.BR TYPE
+.BR netdev
+.BR NETDEV
+
+.ti -8
+.B rdma link delete
+.RI NAME
+
+.ti -8
 .B rdma link help
 
 .SH "DESCRIPTION"
@@ -33,6 +45,31 @@ rdma-link \- rdma link configuration
 - specifies the RDMA link to show.
 If this argument is omitted all links are listed.
 
+.SS rdma link add NAME type TYPE netdev NETDEV - add an rdma link for the specified type to the network device
+.sp
+.BR NAME
+- specifies the new name of the rdma link to add
+
+.BR TYPE
+- specifies which rdma type to use.  Link types:
+.sp
+.in +8
+.B rxe
+- Soft RoCE driver
+.sp
+.B siw
+- Soft iWARP driver
+.in -8
+
+.BR NETDEV
+- specifies the network device to which the link is bound
+
+.SS rdma link delete NAME - delete an rdma link
+.PP
+.BR NAME
+- specifies the name of the rdma link to delete
+.PP
+
 .SH "EXAMPLES"
 .PP
 rdma link show
@@ -45,6 +82,16 @@ rdma link show mlx5_2/1
 Shows the state of specified rdma link.
 .RE
 .PP
+rdma link add rxe_eth0 type rxe netdev eth0
+.RS 4
+Adds a RXE link named rxe_eth0 to network device eth0
+.RE
+.PP
+rdma link del rxe_eth0
+.RS 4
+Removes RXE link rxe_eth0
+.RE
+.PP
 
 .SH SEE ALSO
 .BR rdma (8),
-- 
1.8.3.1

[PATCH v1 iproute2-next 0/4] Dynamic rdma link creation

[Steve Wise]

This series adds rdmatool support for creating/deleting rdma links.
This will be used, mainly, by soft rdma drivers to allow adding/deleting
rdma links over netdev interfaces.  It provides the user side for
the following kernel changes merged in linux-rdma/for-next:

https://www.spinics.net/lists/linux-rdma/msg75609.html

I believe this series is ready to go.  Please review!

Changes since RFC:

- add rd_sendrecv_msg() and make use of it in dev_set as well
  as the new link commands.
- fixed problems with the man pages
- changed the command line to use "netdev" as the keyword
  for the network device, do avoid confused with the ib_device
  name.
- got rid of the "type" parameter for link delete.  Also pass
  down the device index instead of the name, using the common
  rd services for validating the device name and fetching the
  index.

Thanks,

Steve.

Steve Wise (4):
  rdma: add helper rd_sendrecv_msg()
  Sync up rdma_netlink.h
  rdma: add 'link add/delete' commands
  rdma: man page update for link add/delete

 man/man8/rdma-link.8                  | 47 ++++++++++++++++++++++
 rdma/dev.c                            |  2 +-
 rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
 rdma/link.c                           | 67 +++++++++++++++++++++++++++++++
 rdma/rdma.h                           |  2 +
 rdma/utils.c                          | 23 ++++++++++-
 6 files changed, 197 insertions(+), 18 deletions(-)

-- 
1.8.3.1

Re: [PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Stephen Hemminger]

On Thu, 21 Feb 2019 08:19:07 -0800
Steve Wise <swise@opengridcomputing.com> wrote:

> Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
> RDMA_NLDEV_CMD_DELLINK API.
> 
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
> index 04c80cebef49..23a90ad52485 100644
> --- a/rdma/include/uapi/rdma/rdma_netlink.h
> +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> @@ -5,8 +5,7 @@
>  #include <linux/types.h>
>  
>  enum {
> -	RDMA_NL_RDMA_CM = 1,
> -	RDMA_NL_IWCM,
> +	RDMA_NL_IWCM = 2,
>  	RDMA_NL_RSVD,
>  	RDMA_NL_LS,	/* RDMA Local Services */
>  	RDMA_NL_NLDEV,	/* RDMA device interface */

You can't just drop elements from user ABI headers.
That is a break of kernel ABI guarantee.

Instead, mark unused elements:
enum {
	RDMA_NL_RDMA_CM = 1, /* deprecated */
	RDMA_NL_IWCM, 
...

Re: [PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 10:56:31AM -0800, Stephen Hemminger wrote:
> On Thu, 21 Feb 2019 08:19:07 -0800
> Steve Wise <swise@opengridcomputing.com> wrote:
>
> > Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
> > RDMA_NLDEV_CMD_DELLINK API.
> >
> > Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > ---
> >  rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
> >  1 file changed, 58 insertions(+), 16 deletions(-)
> >
> > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
> > index 04c80cebef49..23a90ad52485 100644
> > --- a/rdma/include/uapi/rdma/rdma_netlink.h
> > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> > @@ -5,8 +5,7 @@
> >  #include <linux/types.h>
> >
> >  enum {
> > -	RDMA_NL_RDMA_CM = 1,
> > -	RDMA_NL_IWCM,
> > +	RDMA_NL_IWCM = 2,
> >  	RDMA_NL_RSVD,
> >  	RDMA_NL_LS,	/* RDMA Local Services */
> >  	RDMA_NL_NLDEV,	/* RDMA device interface */
>
> You can't just drop elements from user ABI headers.
> That is a break of kernel ABI guarantee.

In RDMA subsystem, we have less pedantic rules and are removing
unused and mistakenly added code. This removal scheme was suggested
by Linus  - remove and see who is complaining.

The big advantage of RDMA is that we know exactly what is in use
by our consumers.

In this case, we knew for sure that no one is using RDMA_NL_RDMA_CM,
and it never was backed by working user space. More on that,
the talks of RDMA_NL_RDMA_CM removal started a long time ago,
but took time till the patch was actually sent.

So yes, if people use this enum value as subsidiary to 1, they will break.

>
> Instead, mark unused elements:
> enum {
> 	RDMA_NL_RDMA_CM = 1, /* deprecated */
> 	RDMA_NL_IWCM,
> ...

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

Re: [PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Jason Gunthorpe]

On Thu, Feb 21, 2019 at 10:56:31AM -0800, Stephen Hemminger wrote:
> On Thu, 21 Feb 2019 08:19:07 -0800
> Steve Wise <swise@opengridcomputing.com> wrote:
> 
> > Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
> > RDMA_NLDEV_CMD_DELLINK API.
> > 
> > Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >  rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
> >  1 file changed, 58 insertions(+), 16 deletions(-)
> > 
> > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
> > index 04c80cebef49..23a90ad52485 100644
> > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> > @@ -5,8 +5,7 @@
> >  #include <linux/types.h>
> >  
> >  enum {
> > -	RDMA_NL_RDMA_CM = 1,
> > -	RDMA_NL_IWCM,
> > +	RDMA_NL_IWCM = 2,
> >  	RDMA_NL_RSVD,
> >  	RDMA_NL_LS,	/* RDMA Local Services */
> >  	RDMA_NL_NLDEV,	/* RDMA device interface */
> 
> You can't just drop elements from user ABI headers.
> That is a break of kernel ABI guarantee.

The ABI didn't change..

We don't promise unlimited source code compatibility in the uapi
headers. 

If the kernel doesn't support something better to remove all traces of
it so userspace users are aware of the change when their compile
breaks.

Jason

Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Jason Gunthorpe]

On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
> device to a netdev interface.
> 
> EG:
> 
> rdma link add rxe_eth0 type rxe netdev eth0
> rdma link delete rxe_eth0

This is great that we finally got here!

Jason

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> This function sends the constructed netlink message and then
> receives the response, displaying any error text.
>
> Change 'rdma dev set' to use it.
>
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/dev.c   |  2 +-
>  rdma/rdma.h  |  1 +
>  rdma/utils.c | 21 +++++++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/rdma/dev.c b/rdma/dev.c
> index 60ff4b31e320..d2949c378f08 100644
> --- a/rdma/dev.c
> +++ b/rdma/dev.c
> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>
> -	return rd_send_msg(rd);
> +	return rd_sendrecv_msg(rd, seq);
>  }
>
>  static int dev_one_set(struct rd *rd)
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 547bb5749a39..20be2f12c4f8 100644
> --- a/rdma/rdma.h
> +++ b/rdma/rdma.h
> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>   */
>  int rd_send_msg(struct rd *rd);
>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> diff --git a/rdma/utils.c b/rdma/utils.c
> index 069d44fece10..a6f2826c9605 100644
> --- a/rdma/utils.c
> +++ b/rdma/utils.c
> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>  	return ret;
>  }
>
> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> +{
> +	return MNL_CB_OK;
> +}
> +
> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> +{
> +	int ret;
> +
> +	ret = rd_send_msg(rd);
> +	if (ret) {
> +		perror(NULL);

This is more or less already done in rd_send_msg() and that function
prints something in case of execution error. So the missing piece
is to update rd_recv_msg(), so all places will "magically" print errors
and not only dev_set_name().

> +		goto out;
> +	}
> +	ret = rd_recv_msg(rd, null_cb, rd, seq);
> +	if (ret)
> +		perror(NULL);
> +out:
> +	return ret;
> +}
> +
>  static struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name)
>  {
>  	struct dev_map *dev_map;
> --
> 1.8.3.1
>

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

Re: [PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 08:19:07AM -0800, Steve Wise wrote:
> Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
> RDMA_NLDEV_CMD_DELLINK API.
>
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 16 deletions(-)
>

If my series go first, this patch won't be needed.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

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

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Sat, Feb 23, 2019 at 11:26:15AM +0200, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> > This function sends the constructed netlink message and then
> > receives the response, displaying any error text.
> >
> > Change 'rdma dev set' to use it.
> >
> > Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > ---
> >  rdma/dev.c   |  2 +-
> >  rdma/rdma.h  |  1 +
> >  rdma/utils.c | 21 +++++++++++++++++++++
> >  3 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/rdma/dev.c b/rdma/dev.c
> > index 60ff4b31e320..d2949c378f08 100644
> > --- a/rdma/dev.c
> > +++ b/rdma/dev.c
> > @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
> >  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
> >
> > -	return rd_send_msg(rd);
> > +	return rd_sendrecv_msg(rd, seq);
> >  }
> >
> >  static int dev_one_set(struct rd *rd)
> > diff --git a/rdma/rdma.h b/rdma/rdma.h
> > index 547bb5749a39..20be2f12c4f8 100644
> > --- a/rdma/rdma.h
> > +++ b/rdma/rdma.h
> > @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
> >   */
> >  int rd_send_msg(struct rd *rd);
> >  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
> > +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> > diff --git a/rdma/utils.c b/rdma/utils.c
> > index 069d44fece10..a6f2826c9605 100644
> > --- a/rdma/utils.c
> > +++ b/rdma/utils.c
> > @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
> >  	return ret;
> >  }
> >
> > +static int null_cb(const struct nlmsghdr *nlh, void *data)
> > +{
> > +	return MNL_CB_OK;
> > +}
> > +
> > +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> > +{
> > +	int ret;
> > +
> > +	ret = rd_send_msg(rd);
> > +	if (ret) {
> > +		perror(NULL);
>
> This is more or less already done in rd_send_msg() and that function
> prints something in case of execution error. So the missing piece
> is to update rd_recv_msg(), so all places will "magically" print errors
> and not only dev_set_name().
>
> > +		goto out;
> > +	}
> > +	ret = rd_recv_msg(rd, null_cb, rd, seq);

Will this "null_cb" work for all send/recv flows or only in flows where
response can be error only? Will we need this recv_msg if we implement
extack support?

> > +	if (ret)
> > +		perror(NULL);
> > +out:
> > +	return ret;
> > +}
> > +
> >  static struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name)
> >  {
> >  	struct dev_map *dev_map;
> > --
> > 1.8.3.1
> >



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

Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
> device to a netdev interface.
>
> EG:
>
> rdma link add rxe_eth0 type rxe netdev eth0
> rdma link delete rxe_eth0
>
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/link.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rdma/rdma.h  |  1 +
>  rdma/utils.c |  2 +-
>  3 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/rdma/link.c b/rdma/link.c
> index c064be627be2..afaf19663728 100644
> --- a/rdma/link.c
> +++ b/rdma/link.c
> @@ -14,6 +14,9 @@
>  static int link_help(struct rd *rd)
>  {
>  	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
> +	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
> +	       rd->filename);
> +	pr_out("Usage: %s link delete NAME\n", rd->filename);
>  	return 0;
>  }
>
> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
>  	return rd_exec_link(rd, link_one_show, true);
>  }
>
> +static int link_add(struct rd *rd)
> +{
> +	char *name;
> +	char *type = NULL;
> +	char *dev = NULL;
> +	uint32_t seq;
> +
> +	if (rd_no_arg(rd)) {
> +		pr_err("No link name was supplied\n");

I think that it is better to have instruction message and not error
message: "Please provide ...".

> +		return -EINVAL;
> +	}
> +	name = rd_argv(rd);
> +	rd_arg_inc(rd);
> +	while (!rd_no_arg(rd)) {
> +		if (rd_argv_match(rd, "type")) {
> +			rd_arg_inc(rd);
> +			type = rd_argv(rd);
> +		} else if (rd_argv_match(rd, "netdev")) {
> +			rd_arg_inc(rd);
> +			dev = rd_argv(rd);
> +		} else {
> +			pr_err("Invalid parameter %s\n", rd_argv(rd));
> +			return -EINVAL;
> +		}
> +		rd_arg_inc(rd);

Please use chains of struct rd_cmd and rd_exec_cmd() instead of
open-coding parser.

> +	}
> +	if (!type) {
> +		pr_err("No type was supplied\n");
> +		return -EINVAL;

General parser handle it.

> +	}
> +	if (!dev) {
> +		pr_err("No net device was supplied\n");
> +		return -EINVAL;
> +	}

rd_exec_require_dev() ???

> +
> +	rd_prepare_msg(rd, RDMA_NLDEV_CMD_NEWLINK, &seq,
> +		       (NLM_F_REQUEST | NLM_F_ACK));
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_NDEV_NAME, dev);
> +	return rd_sendrecv_msg(rd, seq);
> +}
> +
> +static int _link_del(struct rd *rd)
> +{
> +	uint32_t seq;
> +
> +	if (!rd_no_arg(rd)) {
> +		pr_err("Unknown parameter %s\n", rd_argv(rd));
> +		return -EINVAL;
> +	}
> +	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
> +		       (NLM_F_REQUEST | NLM_F_ACK));
> +	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
> +	return rd_sendrecv_msg(rd, seq);
> +}
> +
> +static int link_del(struct rd *rd)
> +{
> +	return rd_exec_require_dev(rd, _link_del);
> +}
> +
>  int cmd_link(struct rd *rd)
>  {
>  	const struct rd_cmd cmds[] = {
>  		{ NULL,		link_show },
> +		{ "add",	link_add },
> +		{ "delete",	link_del },
>  		{ "show",	link_show },
>  		{ "list",	link_show },
>  		{ "help",	link_help },
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 20be2f12c4f8..af4597f45640 100644
> --- a/rdma/rdma.h
> +++ b/rdma/rdma.h
> @@ -79,6 +79,7 @@ struct rd_cmd {
>   */
>  bool rd_no_arg(struct rd *rd);
>  void rd_arg_inc(struct rd *rd);
> +bool rd_argv_match(struct rd *rd, const char *pattern);
>
>  char *rd_argv(struct rd *rd);
>
> diff --git a/rdma/utils.c b/rdma/utils.c
> index a6f2826c9605..85a954167511 100644
> --- a/rdma/utils.c
> +++ b/rdma/utils.c
> @@ -32,7 +32,7 @@ int strcmpx(const char *str1, const char *str2)
>  	return strncmp(str1, str2, strlen(str1));
>  }
>
> -static bool rd_argv_match(struct rd *rd, const char *pattern)
> +bool rd_argv_match(struct rd *rd, const char *pattern)
>  {
>  	if (!rd_argc(rd))
>  		return false;
> --
> 1.8.3.1
>

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

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
>> This function sends the constructed netlink message and then
>> receives the response, displaying any error text.
>>
>> Change 'rdma dev set' to use it.
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/dev.c   |  2 +-
>>  rdma/rdma.h  |  1 +
>>  rdma/utils.c | 21 +++++++++++++++++++++
>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/dev.c b/rdma/dev.c
>> index 60ff4b31e320..d2949c378f08 100644
>> --- a/rdma/dev.c
>> +++ b/rdma/dev.c
>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>>
>> -	return rd_send_msg(rd);
>> +	return rd_sendrecv_msg(rd, seq);
>>  }
>>
>>  static int dev_one_set(struct rd *rd)
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 547bb5749a39..20be2f12c4f8 100644
>> --- a/rdma/rdma.h
>> +++ b/rdma/rdma.h
>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>>   */
>>  int rd_send_msg(struct rd *rd);
>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
>> diff --git a/rdma/utils.c b/rdma/utils.c
>> index 069d44fece10..a6f2826c9605 100644
>> --- a/rdma/utils.c
>> +++ b/rdma/utils.c
>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>>  	return ret;
>>  }
>>
>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
>> +{
>> +	return MNL_CB_OK;
>> +}
>> +
>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
>> +{
>> +	int ret;
>> +
>> +	ret = rd_send_msg(rd);
>> +	if (ret) {
>> +		perror(NULL);
> This is more or less already done in rd_send_msg() and that function
> prints something in case of execution error. So the missing piece
> is to update rd_recv_msg(), so all places will "magically" print errors
> and not only dev_set_name().

Yea ok.

Re: [PATCH v1 iproute2-next 2/4] Sync up rdma_netlink.h

[Steve Wise]

On 2/23/2019 3:28 AM, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:07AM -0800, Steve Wise wrote:
>> Pull in the latest rdma_netlink.h to get the RDMA_NLDEV_CMD_NEWLINK /
>> RDMA_NLDEV_CMD_DELLINK API.
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/include/uapi/rdma/rdma_netlink.h | 74 +++++++++++++++++++++++++++--------
>>  1 file changed, 58 insertions(+), 16 deletions(-)
>>
> If my series go first, this patch won't be needed.
>
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

Some changes are still needed because your iproute2 series that was
recently merged didn't include the kernel NEWLINK/DELLINK changes.   But
I've rebased this and all the IWPM changes are already there as part of
your series.

Thanks,

Steve.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 2/23/2019 3:31 AM, Leon Romanovsky wrote:
> On Sat, Feb 23, 2019 at 11:26:15AM +0200, Leon Romanovsky wrote:
>> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
>>> This function sends the constructed netlink message and then
>>> receives the response, displaying any error text.
>>>
>>> Change 'rdma dev set' to use it.
>>>
>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>> ---
>>>  rdma/dev.c   |  2 +-
>>>  rdma/rdma.h  |  1 +
>>>  rdma/utils.c | 21 +++++++++++++++++++++
>>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/rdma/dev.c b/rdma/dev.c
>>> index 60ff4b31e320..d2949c378f08 100644
>>> --- a/rdma/dev.c
>>> +++ b/rdma/dev.c
>>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>>>
>>> -	return rd_send_msg(rd);
>>> +	return rd_sendrecv_msg(rd, seq);
>>>  }
>>>
>>>  static int dev_one_set(struct rd *rd)
>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>> index 547bb5749a39..20be2f12c4f8 100644
>>> --- a/rdma/rdma.h
>>> +++ b/rdma/rdma.h
>>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>>>   */
>>>  int rd_send_msg(struct rd *rd);
>>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
>>> diff --git a/rdma/utils.c b/rdma/utils.c
>>> index 069d44fece10..a6f2826c9605 100644
>>> --- a/rdma/utils.c
>>> +++ b/rdma/utils.c
>>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>>>  	return ret;
>>>  }
>>>
>>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
>>> +{
>>> +	return MNL_CB_OK;
>>> +}
>>> +
>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = rd_send_msg(rd);
>>> +	if (ret) {
>>> +		perror(NULL);
>> This is more or less already done in rd_send_msg() and that function
>> prints something in case of execution error. So the missing piece
>> is to update rd_recv_msg(), so all places will "magically" print errors
>> and not only dev_set_name().
>>
>>> +		goto out;
>>> +	}
>>> +	ret = rd_recv_msg(rd, null_cb, rd, seq);
> Will this "null_cb" work for all send/recv flows or only in flows where
> response can be error only? 


Only those flows where no nl attributes are expected to be returned.


> Will we need this recv_msg if we implement
> extack support?


I'm not sure how extack works.  Do you know?

Thanks!

Steve.

Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Steve Wise]

On 2/23/2019 3:43 AM, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
>> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
>> device to a netdev interface.
>>
>> EG:
>>
>> rdma link add rxe_eth0 type rxe netdev eth0
>> rdma link delete rxe_eth0
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/link.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  rdma/rdma.h  |  1 +
>>  rdma/utils.c |  2 +-
>>  3 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/link.c b/rdma/link.c
>> index c064be627be2..afaf19663728 100644
>> --- a/rdma/link.c
>> +++ b/rdma/link.c
>> @@ -14,6 +14,9 @@
>>  static int link_help(struct rd *rd)
>>  {
>>  	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
>> +	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
>> +	       rd->filename);
>> +	pr_out("Usage: %s link delete NAME\n", rd->filename);
>>  	return 0;
>>  }
>>
>> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
>>  	return rd_exec_link(rd, link_one_show, true);
>>  }
>>
>> +static int link_add(struct rd *rd)
>> +{
>> +	char *name;
>> +	char *type = NULL;
>> +	char *dev = NULL;
>> +	uint32_t seq;
>> +
>> +	if (rd_no_arg(rd)) {
>> +		pr_err("No link name was supplied\n");
> I think that it is better to have instruction message and not error
> message: "Please provide ...".


Ok.  Perhaps a new utility rd_exec_require_link() can be created?


>> +		return -EINVAL;
>> +	}
>> +	name = rd_argv(rd);
>> +	rd_arg_inc(rd);
>> +	while (!rd_no_arg(rd)) {
>> +		if (rd_argv_match(rd, "type")) {
>> +			rd_arg_inc(rd);
>> +			type = rd_argv(rd);
>> +		} else if (rd_argv_match(rd, "netdev")) {
>> +			rd_arg_inc(rd);
>> +			dev = rd_argv(rd);
>> +		} else {
>> +			pr_err("Invalid parameter %s\n", rd_argv(rd));
>> +			return -EINVAL;
>> +		}
>> +		rd_arg_inc(rd);
> Please use chains of struct rd_cmd and rd_exec_cmd() instead of
> open-coding parser.


Ok.  Like your recently merged series did...


>> +	}
>> +	if (!type) {
>> +		pr_err("No type was supplied\n");
>> +		return -EINVAL;
> General parser handle it.


Ok.


>> +	}
>> +	if (!dev) {
>> +		pr_err("No net device was supplied\n");
>> +		return -EINVAL;
>> +	}
> rd_exec_require_dev() ???


Looks like I can use that.  I'll try it.


Thanks!


Steve.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Tue, Feb 26, 2019 at 11:19:12AM -0600, Steve Wise wrote:
>
> On 2/23/2019 3:31 AM, Leon Romanovsky wrote:
> > On Sat, Feb 23, 2019 at 11:26:15AM +0200, Leon Romanovsky wrote:
> >> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> >>> This function sends the constructed netlink message and then
> >>> receives the response, displaying any error text.
> >>>
> >>> Change 'rdma dev set' to use it.
> >>>
> >>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >>> ---
> >>>  rdma/dev.c   |  2 +-
> >>>  rdma/rdma.h  |  1 +
> >>>  rdma/utils.c | 21 +++++++++++++++++++++
> >>>  3 files changed, 23 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/rdma/dev.c b/rdma/dev.c
> >>> index 60ff4b31e320..d2949c378f08 100644
> >>> --- a/rdma/dev.c
> >>> +++ b/rdma/dev.c
> >>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
> >>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
> >>>
> >>> -	return rd_send_msg(rd);
> >>> +	return rd_sendrecv_msg(rd, seq);
> >>>  }
> >>>
> >>>  static int dev_one_set(struct rd *rd)
> >>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>> index 547bb5749a39..20be2f12c4f8 100644
> >>> --- a/rdma/rdma.h
> >>> +++ b/rdma/rdma.h
> >>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
> >>>   */
> >>>  int rd_send_msg(struct rd *rd);
> >>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
> >>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> >>> diff --git a/rdma/utils.c b/rdma/utils.c
> >>> index 069d44fece10..a6f2826c9605 100644
> >>> --- a/rdma/utils.c
> >>> +++ b/rdma/utils.c
> >>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
> >>>  	return ret;
> >>>  }
> >>>
> >>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> >>> +{
> >>> +	return MNL_CB_OK;
> >>> +}
> >>> +
> >>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	ret = rd_send_msg(rd);
> >>> +	if (ret) {
> >>> +		perror(NULL);
> >> This is more or less already done in rd_send_msg() and that function
> >> prints something in case of execution error. So the missing piece
> >> is to update rd_recv_msg(), so all places will "magically" print errors
> >> and not only dev_set_name().
> >>
> >>> +		goto out;
> >>> +	}
> >>> +	ret = rd_recv_msg(rd, null_cb, rd, seq);
> > Will this "null_cb" work for all send/recv flows or only in flows where
> > response can be error only?
>
>
> Only those flows where no nl attributes are expected to be returned.
>
>
> > Will we need this recv_msg if we implement
> > extack support?
>
>
> I'm not sure how extack works.  Do you know?

I can't say that :)

>
> Thanks!
>
> Steve.
>
>

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

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 2/26/2019 1:16 PM, Leon Romanovsky wrote:
> On Tue, Feb 26, 2019 at 11:19:12AM -0600, Steve Wise wrote:
>> On 2/23/2019 3:31 AM, Leon Romanovsky wrote:
>>> On Sat, Feb 23, 2019 at 11:26:15AM +0200, Leon Romanovsky wrote:
>>>> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
>>>>> This function sends the constructed netlink message and then
>>>>> receives the response, displaying any error text.
>>>>>
>>>>> Change 'rdma dev set' to use it.
>>>>>
>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>>>> ---
>>>>>  rdma/dev.c   |  2 +-
>>>>>  rdma/rdma.h  |  1 +
>>>>>  rdma/utils.c | 21 +++++++++++++++++++++
>>>>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/rdma/dev.c b/rdma/dev.c
>>>>> index 60ff4b31e320..d2949c378f08 100644
>>>>> --- a/rdma/dev.c
>>>>> +++ b/rdma/dev.c
>>>>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>>>>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>>>>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>>>>>
>>>>> -	return rd_send_msg(rd);
>>>>> +	return rd_sendrecv_msg(rd, seq);
>>>>>  }
>>>>>
>>>>>  static int dev_one_set(struct rd *rd)
>>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>>>> index 547bb5749a39..20be2f12c4f8 100644
>>>>> --- a/rdma/rdma.h
>>>>> +++ b/rdma/rdma.h
>>>>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>>>>>   */
>>>>>  int rd_send_msg(struct rd *rd);
>>>>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>>>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
>>>>> diff --git a/rdma/utils.c b/rdma/utils.c
>>>>> index 069d44fece10..a6f2826c9605 100644
>>>>> --- a/rdma/utils.c
>>>>> +++ b/rdma/utils.c
>>>>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>>>>>  	return ret;
>>>>>  }
>>>>>
>>>>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
>>>>> +{
>>>>> +	return MNL_CB_OK;
>>>>> +}
>>>>> +
>>>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = rd_send_msg(rd);
>>>>> +	if (ret) {
>>>>> +		perror(NULL);
>>>> This is more or less already done in rd_send_msg() and that function
>>>> prints something in case of execution error. So the missing piece
>>>> is to update rd_recv_msg(), so all places will "magically" print errors
>>>> and not only dev_set_name().
>>>>
>>>>> +		goto out;
>>>>> +	}
>>>>> +	ret = rd_recv_msg(rd, null_cb, rd, seq);
>>> Will this "null_cb" work for all send/recv flows or only in flows where
>>> response can be error only?
>>
>> Only those flows where no nl attributes are expected to be returned.
>>
>>
>>> Will we need this recv_msg if we implement
>>> extack support?
>>
>> I'm not sure how extack works.  Do you know?
> I can't say that :)


We can change things if/when we support extack.

Stevo.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[David Ahern]

On 2/26/19 10:19 AM, Steve Wise wrote:
> 
>> Will we need this recv_msg if we implement
>> extack support?
> 
> I'm not sure how extack works.  Do you know?

see devlink/mnlg.c

mnlg_socket_open()
{
	...
	mnl_socket_setsockopt(nlg->nl, NETLINK_EXT_ACK, &one, sizeof(one));
	...
}

and

mnlg_cb_error()

That code under devlink needs to be generic for both tools.

RE: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:14 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 1/4] rdma: add helper
> rd_sendrecv_msg()
> 
> 
> On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> >> This function sends the constructed netlink message and then
> >> receives the response, displaying any error text.
> >>
> >> Change 'rdma dev set' to use it.
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/dev.c   |  2 +-
> >>  rdma/rdma.h  |  1 +
> >>  rdma/utils.c | 21 +++++++++++++++++++++
> >>  3 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/dev.c b/rdma/dev.c
> >> index 60ff4b31e320..d2949c378f08 100644
> >> --- a/rdma/dev.c
> >> +++ b/rdma/dev.c
> >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> >dev_idx);
> >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> rd_argv(rd));
> >>
> >> -	return rd_send_msg(rd);
> >> +	return rd_sendrecv_msg(rd, seq);
> >>  }
> >>
> >>  static int dev_one_set(struct rd *rd)
> >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >> index 547bb5749a39..20be2f12c4f8 100644
> >> --- a/rdma/rdma.h
> >> +++ b/rdma/rdma.h
> >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> char *key);
> >>   */
> >>  int rd_send_msg(struct rd *rd);
> >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t
> seq);
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> >> diff --git a/rdma/utils.c b/rdma/utils.c
> >> index 069d44fece10..a6f2826c9605 100644
> >> --- a/rdma/utils.c
> >> +++ b/rdma/utils.c
> >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
> void *data, unsigned int seq)
> >>  	return ret;
> >>  }
> >>
> >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> >> +{
> >> +	return MNL_CB_OK;
> >> +}
> >> +
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = rd_send_msg(rd);
> >> +	if (ret) {
> >> +		perror(NULL);
> > This is more or less already done in rd_send_msg() and that function
> > prints something in case of execution error. So the missing piece
> > is to update rd_recv_msg(), so all places will "magically" print errors
> > and not only dev_set_name().
> 
> Yea ok.
> 

dev_set_name() doesn't call rd_recv_msg().  So you're suggesting I fix up
rd_recv_msg() to display errors and make dev_set_name() call rd_recv_msg()
with the null_cb function?  You sure that's the way to go?

RE: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:14 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 1/4] rdma: add helper
> rd_sendrecv_msg()
> 
> 
> On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> >> This function sends the constructed netlink message and then
> >> receives the response, displaying any error text.
> >>
> >> Change 'rdma dev set' to use it.
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/dev.c   |  2 +-
> >>  rdma/rdma.h  |  1 +
> >>  rdma/utils.c | 21 +++++++++++++++++++++
> >>  3 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/dev.c b/rdma/dev.c
> >> index 60ff4b31e320..d2949c378f08 100644
> >> --- a/rdma/dev.c
> >> +++ b/rdma/dev.c
> >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> >dev_idx);
> >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> rd_argv(rd));
> >>
> >> -	return rd_send_msg(rd);
> >> +	return rd_sendrecv_msg(rd, seq);
> >>  }
> >>
> >>  static int dev_one_set(struct rd *rd)
> >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >> index 547bb5749a39..20be2f12c4f8 100644
> >> --- a/rdma/rdma.h
> >> +++ b/rdma/rdma.h
> >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> char *key);
> >>   */
> >>  int rd_send_msg(struct rd *rd);
> >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t
> seq);
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> >> diff --git a/rdma/utils.c b/rdma/utils.c
> >> index 069d44fece10..a6f2826c9605 100644
> >> --- a/rdma/utils.c
> >> +++ b/rdma/utils.c
> >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
> void *data, unsigned int seq)
> >>  	return ret;
> >>  }
> >>
> >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> >> +{
> >> +	return MNL_CB_OK;
> >> +}
> >> +
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = rd_send_msg(rd);
> >> +	if (ret) {
> >> +		perror(NULL);
> > This is more or less already done in rd_send_msg() and that function
> > prints something in case of execution error. So the missing piece
> > is to update rd_recv_msg(), so all places will "magically" print errors
> > and not only dev_set_name().
> 
> Yea ok.
> 

dev_set_name() doesn't call rd_recv_msg().  So you're suggesting I fix up
rd_recv_msg() to display errors and make dev_set_name() call rd_recv_msg()
with the null_cb function?  You sure that's the way to go?

RE: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Steve Wise]

> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:25 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete'
> commands
> 
> 
> On 2/23/2019 3:43 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
> >> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-
> rdma
> >> device to a netdev interface.
> >>
> >> EG:
> >>
> >> rdma link add rxe_eth0 type rxe netdev eth0
> >> rdma link delete rxe_eth0
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/link.c  | 67
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  rdma/rdma.h  |  1 +
> >>  rdma/utils.c |  2 +-
> >>  3 files changed, 69 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/link.c b/rdma/link.c
> >> index c064be627be2..afaf19663728 100644
> >> --- a/rdma/link.c
> >> +++ b/rdma/link.c
> >> @@ -14,6 +14,9 @@
> >>  static int link_help(struct rd *rd)
> >>  {
> >>  	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
> >> +	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
> >> +	       rd->filename);
> >> +	pr_out("Usage: %s link delete NAME\n", rd->filename);
> >>  	return 0;
> >>  }
> >>
> >> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
> >>  	return rd_exec_link(rd, link_one_show, true);
> >>  }
> >>
> >> +static int link_add(struct rd *rd)
> >> +{
> >> +	char *name;
> >> +	char *type = NULL;
> >> +	char *dev = NULL;
> >> +	uint32_t seq;
> >> +
> >> +	if (rd_no_arg(rd)) {
> >> +		pr_err("No link name was supplied\n");
> > I think that it is better to have instruction message and not error
> > message: "Please provide ...".
> 
> 
> Ok.  Perhaps a new utility rd_exec_require_link() can be created?
> 
> 
> >> +		return -EINVAL;
> >> +	}
> >> +	name = rd_argv(rd);
> >> +	rd_arg_inc(rd);
> >> +	while (!rd_no_arg(rd)) {
> >> +		if (rd_argv_match(rd, "type")) {
> >> +			rd_arg_inc(rd);
> >> +			type = rd_argv(rd);
> >> +		} else if (rd_argv_match(rd, "netdev")) {
> >> +			rd_arg_inc(rd);
> >> +			dev = rd_argv(rd);
> >> +		} else {
> >> +			pr_err("Invalid parameter %s\n", rd_argv(rd));
> >> +			return -EINVAL;
> >> +		}
> >> +		rd_arg_inc(rd);
> > Please use chains of struct rd_cmd and rd_exec_cmd() instead of
> > open-coding parser.
> 
> 
> Ok.  Like your recently merged series did...
> 
> 
> >> +	}
> >> +	if (!type) {
> >> +		pr_err("No type was supplied\n");
> >> +		return -EINVAL;
> > General parser handle it.
> 
> 
> Ok.
> 
> 
> >> +	}
> >> +	if (!dev) {
> >> +		pr_err("No net device was supplied\n");
> >> +		return -EINVAL;
> >> +	}
> > rd_exec_require_dev() ???
> 
> 
> Looks like I can use that.  I'll try it.

Actually, in the above code, the variable 'dev' is the netdev string, and
that is the 3rd parameter to the 'link add' command, so it isn't appropriate
for rd_exec_require_dev().

Steve

RE: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands

[Steve Wise]

> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:25 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete'
> commands
> 
> 
> On 2/23/2019 3:43 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
> >> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-
> rdma
> >> device to a netdev interface.
> >>
> >> EG:
> >>
> >> rdma link add rxe_eth0 type rxe netdev eth0
> >> rdma link delete rxe_eth0
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/link.c  | 67
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  rdma/rdma.h  |  1 +
> >>  rdma/utils.c |  2 +-
> >>  3 files changed, 69 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/link.c b/rdma/link.c
> >> index c064be627be2..afaf19663728 100644
> >> --- a/rdma/link.c
> >> +++ b/rdma/link.c
> >> @@ -14,6 +14,9 @@
> >>  static int link_help(struct rd *rd)
> >>  {
> >>  	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
> >> +	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
> >> +	       rd->filename);
> >> +	pr_out("Usage: %s link delete NAME\n", rd->filename);
> >>  	return 0;
> >>  }
> >>
> >> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
> >>  	return rd_exec_link(rd, link_one_show, true);
> >>  }
> >>
> >> +static int link_add(struct rd *rd)
> >> +{
> >> +	char *name;
> >> +	char *type = NULL;
> >> +	char *dev = NULL;
> >> +	uint32_t seq;
> >> +
> >> +	if (rd_no_arg(rd)) {
> >> +		pr_err("No link name was supplied\n");
> > I think that it is better to have instruction message and not error
> > message: "Please provide ...".
> 
> 
> Ok.  Perhaps a new utility rd_exec_require_link() can be created?
> 
> 
> >> +		return -EINVAL;
> >> +	}
> >> +	name = rd_argv(rd);
> >> +	rd_arg_inc(rd);
> >> +	while (!rd_no_arg(rd)) {
> >> +		if (rd_argv_match(rd, "type")) {
> >> +			rd_arg_inc(rd);
> >> +			type = rd_argv(rd);
> >> +		} else if (rd_argv_match(rd, "netdev")) {
> >> +			rd_arg_inc(rd);
> >> +			dev = rd_argv(rd);
> >> +		} else {
> >> +			pr_err("Invalid parameter %s\n", rd_argv(rd));
> >> +			return -EINVAL;
> >> +		}
> >> +		rd_arg_inc(rd);
> > Please use chains of struct rd_cmd and rd_exec_cmd() instead of
> > open-coding parser.
> 
> 
> Ok.  Like your recently merged series did...
> 
> 
> >> +	}
> >> +	if (!type) {
> >> +		pr_err("No type was supplied\n");
> >> +		return -EINVAL;
> > General parser handle it.
> 
> 
> Ok.
> 
> 
> >> +	}
> >> +	if (!dev) {
> >> +		pr_err("No net device was supplied\n");
> >> +		return -EINVAL;
> >> +	}
> > rd_exec_require_dev() ???
> 
> 
> Looks like I can use that.  I'll try it.

Actually, in the above code, the variable 'dev' is the netdev string, and
that is the 3rd parameter to the 'link add' command, so it isn't appropriate
for rd_exec_require_dev().

Steve

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
>> This function sends the constructed netlink message and then
>> receives the response, displaying any error text.
>>
>> Change 'rdma dev set' to use it.
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/dev.c   |  2 +-
>>  rdma/rdma.h  |  1 +
>>  rdma/utils.c | 21 +++++++++++++++++++++
>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/dev.c b/rdma/dev.c
>> index 60ff4b31e320..d2949c378f08 100644
>> --- a/rdma/dev.c
>> +++ b/rdma/dev.c
>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>>
>> -	return rd_send_msg(rd);
>> +	return rd_sendrecv_msg(rd, seq);
>>  }
>>
>>  static int dev_one_set(struct rd *rd)
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 547bb5749a39..20be2f12c4f8 100644
>> --- a/rdma/rdma.h
>> +++ b/rdma/rdma.h
>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>>   */
>>  int rd_send_msg(struct rd *rd);
>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
>> diff --git a/rdma/utils.c b/rdma/utils.c
>> index 069d44fece10..a6f2826c9605 100644
>> --- a/rdma/utils.c
>> +++ b/rdma/utils.c
>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>>  	return ret;
>>  }
>>
>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
>> +{
>> +	return MNL_CB_OK;
>> +}
>> +
>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
>> +{
>> +	int ret;
>> +
>> +	ret = rd_send_msg(rd);
>> +	if (ret) {
>> +		perror(NULL);
> This is more or less already done in rd_send_msg() and that function
> prints something in case of execution error. So the missing piece
> is to update rd_recv_msg(), so all places will "magically" print errors
> and not only dev_set_name().

Hey Leon,

Displaying errors in rd_recv_msg() as you suggested:

@@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
void *data, unsigned int seq)
                ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
        } while (ret > 0);

+       if (ret < 0)
+               perror(NULL);
+
        mnl_socket_close(rd->nl);
        return ret;
 }

Causes problems when doing filtered dumps:

[root@stevo1 iproute2]# ./rdma/rdma res show qp
link mlx5_0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
link mlx5_1/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
link mlx5_1/1 lqpn 2224 type UD state RTS sq-psn 0 comm [ib_core]
link rxe_foo0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
[root@stevo1 iproute2]# ./rdma/rdma res show qp  lqpn 2224
Invalid argument
No such file or directory
Invalid argument
link mlx5_1/1 lqpn 2224 type UD state RTS sq-psn 0 comm [ib_core]
Invalid argument
No such file or directory
[root@stevo1 iproute2]#


Steve.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Thu, Feb 28, 2019 at 01:41:20PM -0600, Steve Wise wrote:
>
> On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> >> This function sends the constructed netlink message and then
> >> receives the response, displaying any error text.
> >>
> >> Change 'rdma dev set' to use it.
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/dev.c   |  2 +-
> >>  rdma/rdma.h  |  1 +
> >>  rdma/utils.c | 21 +++++++++++++++++++++
> >>  3 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/dev.c b/rdma/dev.c
> >> index 60ff4b31e320..d2949c378f08 100644
> >> --- a/rdma/dev.c
> >> +++ b/rdma/dev.c
> >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
> >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
> >>
> >> -	return rd_send_msg(rd);
> >> +	return rd_sendrecv_msg(rd, seq);
> >>  }
> >>
> >>  static int dev_one_set(struct rd *rd)
> >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >> index 547bb5749a39..20be2f12c4f8 100644
> >> --- a/rdma/rdma.h
> >> +++ b/rdma/rdma.h
> >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
> >>   */
> >>  int rd_send_msg(struct rd *rd);
> >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> >> diff --git a/rdma/utils.c b/rdma/utils.c
> >> index 069d44fece10..a6f2826c9605 100644
> >> --- a/rdma/utils.c
> >> +++ b/rdma/utils.c
> >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
> >>  	return ret;
> >>  }
> >>
> >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> >> +{
> >> +	return MNL_CB_OK;
> >> +}
> >> +
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = rd_send_msg(rd);
> >> +	if (ret) {
> >> +		perror(NULL);
> > This is more or less already done in rd_send_msg() and that function
> > prints something in case of execution error. So the missing piece
> > is to update rd_recv_msg(), so all places will "magically" print errors
> > and not only dev_set_name().
>
> Hey Leon,
>
> Displaying errors in rd_recv_msg() as you suggested:
>
> @@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
> void *data, unsigned int seq)
>                 ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
>         } while (ret > 0);
>
> +       if (ret < 0)
> +               perror(NULL);
> +
>         mnl_socket_close(rd->nl);
>         return ret;
>  }
>
> Causes problems when doing filtered dumps:
>
> [root@stevo1 iproute2]# ./rdma/rdma res show qp
> link mlx5_0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
> link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
> link mlx5_1/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
> link mlx5_1/1 lqpn 2224 type UD state RTS sq-psn 0 comm [ib_core]
> link rxe_foo0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
> [root@stevo1 iproute2]# ./rdma/rdma res show qp  lqpn 2224
> Invalid argument
> No such file or directory

Why do we it? We are supposed to check "invalid argument" before sending
message and are not supposed to see perror(). I'm not near code right
now, so most probably wrong in my assumption.

> Invalid argument
> link mlx5_1/1 lqpn 2224 type UD state RTS sq-psn 0 comm [ib_core]
> Invalid argument
> No such file or directory
> [root@stevo1 iproute2]#
>
>
> Steve.
>

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

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 2/28/2019 1:56 PM, Leon Romanovsky wrote:
> On Thu, Feb 28, 2019 at 01:41:20PM -0600, Steve Wise wrote:
>> On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
>>> On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
>>>> This function sends the constructed netlink message and then
>>>> receives the response, displaying any error text.
>>>>
>>>> Change 'rdma dev set' to use it.
>>>>
>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>>> ---
>>>>  rdma/dev.c   |  2 +-
>>>>  rdma/rdma.h  |  1 +
>>>>  rdma/utils.c | 21 +++++++++++++++++++++
>>>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/rdma/dev.c b/rdma/dev.c
>>>> index 60ff4b31e320..d2949c378f08 100644
>>>> --- a/rdma/dev.c
>>>> +++ b/rdma/dev.c
>>>> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
>>>>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
>>>>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, rd_argv(rd));
>>>>
>>>> -	return rd_send_msg(rd);
>>>> +	return rd_sendrecv_msg(rd, seq);
>>>>  }
>>>>
>>>>  static int dev_one_set(struct rd *rd)
>>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>>>> index 547bb5749a39..20be2f12c4f8 100644
>>>> --- a/rdma/rdma.h
>>>> +++ b/rdma/rdma.h
>>>> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const char *key);
>>>>   */
>>>>  int rd_send_msg(struct rd *rd);
>>>>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq);
>>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
>>>> diff --git a/rdma/utils.c b/rdma/utils.c
>>>> index 069d44fece10..a6f2826c9605 100644
>>>> --- a/rdma/utils.c
>>>> +++ b/rdma/utils.c
>>>> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
>>>>  	return ret;
>>>>  }
>>>>
>>>> +static int null_cb(const struct nlmsghdr *nlh, void *data)
>>>> +{
>>>> +	return MNL_CB_OK;
>>>> +}
>>>> +
>>>> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = rd_send_msg(rd);
>>>> +	if (ret) {
>>>> +		perror(NULL);
>>> This is more or less already done in rd_send_msg() and that function
>>> prints something in case of execution error. So the missing piece
>>> is to update rd_recv_msg(), so all places will "magically" print errors
>>> and not only dev_set_name().
>> Hey Leon,
>>
>> Displaying errors in rd_recv_msg() as you suggested:
>>
>> @@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
>> void *data, unsigned int seq)
>>                 ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
>>         } while (ret > 0);
>>
>> +       if (ret < 0)
>> +               perror(NULL);
>> +
>>         mnl_socket_close(rd->nl);
>>         return ret;
>>  }
>>
>> Causes problems when doing filtered dumps:
>>
>> [root@stevo1 iproute2]# ./rdma/rdma res show qp
>> link mlx5_0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
>> link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
>> link mlx5_1/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
>> link mlx5_1/1 lqpn 2224 type UD state RTS sq-psn 0 comm [ib_core]
>> link rxe_foo0/1 lqpn 1 type GSI state RTS sq-psn 0 comm [ib_core]
>> [root@stevo1 iproute2]# ./rdma/rdma res show qp  lqpn 2224
>> Invalid argument
>> No such file or directory
> Why do we it? We are supposed to check "invalid argument" before sending
> message and are not supposed to see perror(). I'm not near code right
> now, so most probably wrong in my assumption.

I'm still investigating, but I _think_ it is because mnl_run_cb(),
called by rd_recv_msg() returns the return code from the callback
function.  So the resource callback functions must be returning an error
when a returned resource doesn't match the filter.  Maybe.  It is
related to doing a rdma res dump where you specify a filter...

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Wed, Feb 27, 2019 at 02:23:45PM -0600, Steve Wise wrote:
>
>
> > -----Original Message-----
> > From: Steve Wise <swise@opengridcomputing.com>
> > Sent: Tuesday, February 26, 2019 11:14 AM
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: dsahern@gmail.com; stephen@networkplumber.org;
> > netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> > Subject: Re: [PATCH v1 iproute2-next 1/4] rdma: add helper
> > rd_sendrecv_msg()
> >
> >
> > On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> > >> This function sends the constructed netlink message and then
> > >> receives the response, displaying any error text.
> > >>
> > >> Change 'rdma dev set' to use it.
> > >>
> > >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > >> ---
> > >>  rdma/dev.c   |  2 +-
> > >>  rdma/rdma.h  |  1 +
> > >>  rdma/utils.c | 21 +++++++++++++++++++++
> > >>  3 files changed, 23 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/rdma/dev.c b/rdma/dev.c
> > >> index 60ff4b31e320..d2949c378f08 100644
> > >> --- a/rdma/dev.c
> > >> +++ b/rdma/dev.c
> > >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> > >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> > >dev_idx);
> > >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> > rd_argv(rd));
> > >>
> > >> -	return rd_send_msg(rd);
> > >> +	return rd_sendrecv_msg(rd, seq);
> > >>  }
> > >>
> > >>  static int dev_one_set(struct rd *rd)
> > >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > >> index 547bb5749a39..20be2f12c4f8 100644
> > >> --- a/rdma/rdma.h
> > >> +++ b/rdma/rdma.h
> > >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> > char *key);
> > >>   */
> > >>  int rd_send_msg(struct rd *rd);
> > >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t
> > seq);
> > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> > >> diff --git a/rdma/utils.c b/rdma/utils.c
> > >> index 069d44fece10..a6f2826c9605 100644
> > >> --- a/rdma/utils.c
> > >> +++ b/rdma/utils.c
> > >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
> > void *data, unsigned int seq)
> > >>  	return ret;
> > >>  }
> > >>
> > >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> > >> +{
> > >> +	return MNL_CB_OK;
> > >> +}
> > >> +
> > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> > >> +{
> > >> +	int ret;
> > >> +
> > >> +	ret = rd_send_msg(rd);
> > >> +	if (ret) {
> > >> +		perror(NULL);
> > > This is more or less already done in rd_send_msg() and that function
> > > prints something in case of execution error. So the missing piece
> > > is to update rd_recv_msg(), so all places will "magically" print errors
> > > and not only dev_set_name().
> >
> > Yea ok.
> >
>
> dev_set_name() doesn't call rd_recv_msg().  So you're suggesting I fix up
> rd_recv_msg() to display errors and make dev_set_name() call rd_recv_msg()
> with the null_cb function?  You sure that's the way to go?

I'm sure that we need to fix dev_set_name(), everything else I'm not sure.

Thanks

>
>
>
>

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

RE: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

> > >
> > > On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > > > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> > > >> This function sends the constructed netlink message and then
> > > >> receives the response, displaying any error text.
> > > >>
> > > >> Change 'rdma dev set' to use it.
> > > >>
> > > >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > > >> ---
> > > >>  rdma/dev.c   |  2 +-
> > > >>  rdma/rdma.h  |  1 +
> > > >>  rdma/utils.c | 21 +++++++++++++++++++++
> > > >>  3 files changed, 23 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/rdma/dev.c b/rdma/dev.c
> > > >> index 60ff4b31e320..d2949c378f08 100644
> > > >> --- a/rdma/dev.c
> > > >> +++ b/rdma/dev.c
> > > >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> > > >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> > > >dev_idx);
> > > >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> > > rd_argv(rd));
> > > >>
> > > >> -	return rd_send_msg(rd);
> > > >> +	return rd_sendrecv_msg(rd, seq);
> > > >>  }
> > > >>
> > > >>  static int dev_one_set(struct rd *rd)
> > > >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > > >> index 547bb5749a39..20be2f12c4f8 100644
> > > >> --- a/rdma/rdma.h
> > > >> +++ b/rdma/rdma.h
> > > >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> > > char *key);
> > > >>   */
> > > >>  int rd_send_msg(struct rd *rd);
> > > >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data,
uint32_t
> > > seq);
> > > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> > > >> diff --git a/rdma/utils.c b/rdma/utils.c
> > > >> index 069d44fece10..a6f2826c9605 100644
> > > >> --- a/rdma/utils.c
> > > >> +++ b/rdma/utils.c
> > > >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t
> callback,
> > > void *data, unsigned int seq)
> > > >>  	return ret;
> > > >>  }
> > > >>
> > > >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> > > >> +{
> > > >> +	return MNL_CB_OK;
> > > >> +}
> > > >> +
> > > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> > > >> +{
> > > >> +	int ret;
> > > >> +
> > > >> +	ret = rd_send_msg(rd);
> > > >> +	if (ret) {
> > > >> +		perror(NULL);
> > > > This is more or less already done in rd_send_msg() and that function
> > > > prints something in case of execution error. So the missing piece
> > > > is to update rd_recv_msg(), so all places will "magically" print
errors
> > > > and not only dev_set_name().
> > >
> > > Yea ok.
> > >
> >
> > dev_set_name() doesn't call rd_recv_msg().  So you're suggesting I fix
up
> > rd_recv_msg() to display errors and make dev_set_name() call
> rd_recv_msg()
> > with the null_cb function?  You sure that's the way to go?
> 
> I'm sure that we need to fix dev_set_name(), everything else I'm not sure.
> 
> Thanks

Hey Leon, adding this to rd_recv_msg():

@@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void
*data, unsigned int seq)
                ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
        } while (ret > 0);

+       if (ret < 0)
+               perror(NULL);
+
        mnl_socket_close(rd->nl);
        return ret;
 }

Results in unexpected errors being logged when doing a query such as:

[root@stevo1 iproute2]# ./rdma/rdma res show qp lqpn 176
error: Invalid argument
link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
error: Invalid argument
error: No such file or directory
error: Invalid argument
error: No such file or directory

It appears the "invalid argument" errors are due to rdmatool sending a
RDMA_NLDEV_CMD_RES_QP_GET command using the doit kernel method to allow
querying for just a QP with lqpn = 176.  However, rdmatool isn't passing a
port index in the messages that generate the "invalid argument" error from
the kernel.  IE you must provide a device index and port index when issuing
a doit command vs a dumpit command.  I think. 

This error was not found because rd_recv_msg() never displayed any errors
previously.  Further, the RES_FUNC() massive macro has code that will retry
a failed doit call with a dumpit call.  I think _##name() should distinguish
between failures reported by the kernel doit function vs failures because no
doit function exists.  Not sure how to support that.


        static inline int _##name(struct rd *rd)
\
        {
\
                uint32_t idx;
\
                int ret;
\
                if (id) {
\
                        ret = rd_doit_index(rd, &idx);
\
                        if (ret) {
\
                                ret = _res_send_idx_msg(rd, command,
\
                                                        name##_idx_parse_cb,
\
                                                        idx, id);
\
                                if (!ret)
\
                                        return ret;
\
                                /* Fallback for old systems without .doit
callbacks */ \
                        }
\
                }
\
                return _res_send_msg(rd, command, name##_parse_cb);
\
        }
\



The "no such file or dir" errors are being returned because, in my setup,
there are 2 other links that do not have lqpn 176.   So there are 2 issues
uncovered by adding generic printing of errors in rd_recv_msg()

1) the doit code in rdmatool is generating requests for a doit method in the
kernel w/o providing a port index.
2) some paths in rdmatool should not print "benign" errors like filtering on
a GET command causing a "does not exist" error returned by the kernel doit
func.

#1 is a bug, IMO.  Can you propose a fix?
#2 could be solved by adding an error callback func passed to rd_recv_msg().
Then the RES_FUNC() functions could parse errors like "no such file or dir"
when doing a filtered query and silently drop them.  And functions like
dev_set_name() would display all errors returned because there are no
expected errors other than "success".

Steve.

RE: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

> > >
> > > On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > > > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> > > >> This function sends the constructed netlink message and then
> > > >> receives the response, displaying any error text.
> > > >>
> > > >> Change 'rdma dev set' to use it.
> > > >>
> > > >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > > >> ---
> > > >>  rdma/dev.c   |  2 +-
> > > >>  rdma/rdma.h  |  1 +
> > > >>  rdma/utils.c | 21 +++++++++++++++++++++
> > > >>  3 files changed, 23 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/rdma/dev.c b/rdma/dev.c
> > > >> index 60ff4b31e320..d2949c378f08 100644
> > > >> --- a/rdma/dev.c
> > > >> +++ b/rdma/dev.c
> > > >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> > > >>  	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> > > >dev_idx);
> > > >>  	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> > > rd_argv(rd));
> > > >>
> > > >> -	return rd_send_msg(rd);
> > > >> +	return rd_sendrecv_msg(rd, seq);
> > > >>  }
> > > >>
> > > >>  static int dev_one_set(struct rd *rd)
> > > >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> > > >> index 547bb5749a39..20be2f12c4f8 100644
> > > >> --- a/rdma/rdma.h
> > > >> +++ b/rdma/rdma.h
> > > >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> > > char *key);
> > > >>   */
> > > >>  int rd_send_msg(struct rd *rd);
> > > >>  int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data,
uint32_t
> > > seq);
> > > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int 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);
> > > >> diff --git a/rdma/utils.c b/rdma/utils.c
> > > >> index 069d44fece10..a6f2826c9605 100644
> > > >> --- a/rdma/utils.c
> > > >> +++ b/rdma/utils.c
> > > >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t
> callback,
> > > void *data, unsigned int seq)
> > > >>  	return ret;
> > > >>  }
> > > >>
> > > >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> > > >> +{
> > > >> +	return MNL_CB_OK;
> > > >> +}
> > > >> +
> > > >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> > > >> +{
> > > >> +	int ret;
> > > >> +
> > > >> +	ret = rd_send_msg(rd);
> > > >> +	if (ret) {
> > > >> +		perror(NULL);
> > > > This is more or less already done in rd_send_msg() and that function
> > > > prints something in case of execution error. So the missing piece
> > > > is to update rd_recv_msg(), so all places will "magically" print
errors
> > > > and not only dev_set_name().
> > >
> > > Yea ok.
> > >
> >
> > dev_set_name() doesn't call rd_recv_msg().  So you're suggesting I fix
up
> > rd_recv_msg() to display errors and make dev_set_name() call
> rd_recv_msg()
> > with the null_cb function?  You sure that's the way to go?
> 
> I'm sure that we need to fix dev_set_name(), everything else I'm not sure.
> 
> Thanks

Hey Leon, adding this to rd_recv_msg():

@@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void
*data, unsigned int seq)
                ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
        } while (ret > 0);

+       if (ret < 0)
+               perror(NULL);
+
        mnl_socket_close(rd->nl);
        return ret;
 }

Results in unexpected errors being logged when doing a query such as:

[root@stevo1 iproute2]# ./rdma/rdma res show qp lqpn 176
error: Invalid argument
link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
error: Invalid argument
error: No such file or directory
error: Invalid argument
error: No such file or directory

It appears the "invalid argument" errors are due to rdmatool sending a
RDMA_NLDEV_CMD_RES_QP_GET command using the doit kernel method to allow
querying for just a QP with lqpn = 176.  However, rdmatool isn't passing a
port index in the messages that generate the "invalid argument" error from
the kernel.  IE you must provide a device index and port index when issuing
a doit command vs a dumpit command.  I think. 

This error was not found because rd_recv_msg() never displayed any errors
previously.  Further, the RES_FUNC() massive macro has code that will retry
a failed doit call with a dumpit call.  I think _##name() should distinguish
between failures reported by the kernel doit function vs failures because no
doit function exists.  Not sure how to support that.


        static inline int _##name(struct rd *rd)
\
        {
\
                uint32_t idx;
\
                int ret;
\
                if (id) {
\
                        ret = rd_doit_index(rd, &idx);
\
                        if (ret) {
\
                                ret = _res_send_idx_msg(rd, command,
\
                                                        name##_idx_parse_cb,
\
                                                        idx, id);
\
                                if (!ret)
\
                                        return ret;
\
                                /* Fallback for old systems without .doit
callbacks */ \
                        }
\
                }
\
                return _res_send_msg(rd, command, name##_parse_cb);
\
        }
\



The "no such file or dir" errors are being returned because, in my setup,
there are 2 other links that do not have lqpn 176.   So there are 2 issues
uncovered by adding generic printing of errors in rd_recv_msg()

1) the doit code in rdmatool is generating requests for a doit method in the
kernel w/o providing a port index.
2) some paths in rdmatool should not print "benign" errors like filtering on
a GET command causing a "does not exist" error returned by the kernel doit
func.

#1 is a bug, IMO.  Can you propose a fix?
#2 could be solved by adding an error callback func passed to rd_recv_msg().
Then the RES_FUNC() functions could parse errors like "no such file or dir"
when doing a filtered query and silently drop them.  And functions like
dev_set_name() would display all errors returned because there are no
expected errors other than "success".

Steve.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Steve Wise]

On 3/4/2019 8:13 AM, Steve Wise wrote:
> Hey Leon, adding this to rd_recv_msg():
>
> @@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void
> *data, unsigned int seq)
>                 ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
>         } while (ret > 0);
>
> +       if (ret < 0)
> +               perror(NULL);
> +
>         mnl_socket_close(rd->nl);
>         return ret;
>  }
>
> Results in unexpected errors being logged when doing a query such as:
>
> [root@stevo1 iproute2]# ./rdma/rdma res show qp lqpn 176
> error: Invalid argument
> link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
> error: Invalid argument
> error: No such file or directory
> error: Invalid argument
> error: No such file or directory
>
> It appears the "invalid argument" errors are due to rdmatool sending a
> RDMA_NLDEV_CMD_RES_QP_GET command using the doit kernel method to allow
> querying for just a QP with lqpn = 176.  However, rdmatool isn't passing a
> port index in the messages that generate the "invalid argument" error from
> the kernel.  IE you must provide a device index and port index when issuing
> a doit command vs a dumpit command.  I think. 
>
> This error was not found because rd_recv_msg() never displayed any errors
> previously.  Further, the RES_FUNC() massive macro has code that will retry
> a failed doit call with a dumpit call.  I think _##name() should distinguish
> between failures reported by the kernel doit function vs failures because no
> doit function exists.  Not sure how to support that.
>
>
>         static inline int _##name(struct rd *rd)
> \
>         {
> \
>                 uint32_t idx;
> \
>                 int ret;
> \
>                 if (id) {
> \
>                         ret = rd_doit_index(rd, &idx);
> \
>                         if (ret) {
> \
>                                 ret = _res_send_idx_msg(rd, command,
> \
>                                                         name##_idx_parse_cb,
> \
>                                                         idx, id);
> \
>                                 if (!ret)
> \
>                                         return ret;
> \
>                                 /* Fallback for old systems without .doit
> callbacks */ \
>                         }
> \
>                 }
> \
>                 return _res_send_msg(rd, command, name##_parse_cb);
> \
>         }
> \
>
>
>
> The "no such file or dir" errors are being returned because, in my setup,
> there are 2 other links that do not have lqpn 176.   So there are 2 issues
> uncovered by adding generic printing of errors in rd_recv_msg()
>
> 1) the doit code in rdmatool is generating requests for a doit method in the
> kernel w/o providing a port index.
> 2) some paths in rdmatool should not print "benign" errors like filtering on
> a GET command causing a "does not exist" error returned by the kernel doit
> func.
>
> #1 is a bug, IMO.  Can you propose a fix?
> #2 could be solved by adding an error callback func passed to rd_recv_msg().
> Then the RES_FUNC() functions could parse errors like "no such file or dir"
> when doing a filtered query and silently drop them.  And functions like
> dev_set_name() would display all errors returned because there are no
> expected errors other than "success".
>
> Steve.
>

Hey Leon, you've been quiet. :)   Thoughts?

Thanks,

Steve.

Re: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()

[Leon Romanovsky]

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

On Wed, Mar 06, 2019 at 03:50:13PM -0600, Steve Wise wrote:
>
> On 3/4/2019 8:13 AM, Steve Wise wrote:
> > Hey Leon, adding this to rd_recv_msg():
> >
> > @@ -693,10 +693,28 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void
> > *data, unsigned int seq)
> >                 ret = mnl_cb_run(buf, ret, seq, portid, callback, data);
> >         } while (ret > 0);
> >
> > +       if (ret < 0)
> > +               perror(NULL);
> > +
> >         mnl_socket_close(rd->nl);
> >         return ret;
> >  }
> >
> > Results in unexpected errors being logged when doing a query such as:
> >
> > [root@stevo1 iproute2]# ./rdma/rdma res show qp lqpn 176
> > error: Invalid argument
> > link mlx5_0/1 lqpn 176 type UD state RTS sq-psn 0 comm [ib_core]
> > error: Invalid argument
> > error: No such file or directory
> > error: Invalid argument
> > error: No such file or directory
> >
> > It appears the "invalid argument" errors are due to rdmatool sending a
> > RDMA_NLDEV_CMD_RES_QP_GET command using the doit kernel method to allow
> > querying for just a QP with lqpn = 176.  However, rdmatool isn't passing a
> > port index in the messages that generate the "invalid argument" error from
> > the kernel.  IE you must provide a device index and port index when issuing
> > a doit command vs a dumpit command.  I think.

QPs are per-device and not per-port, so I can't issue "real" port on
multi-port devices.

> >
> > This error was not found because rd_recv_msg() never displayed any errors
> > previously.  Further, the RES_FUNC() massive macro has code that will retry
> > a failed doit call with a dumpit call.  I think _##name() should distinguish
> > between failures reported by the kernel doit function vs failures because no
> > doit function exists.  Not sure how to support that.

We can suppress prints from failures to find .doit, by adding extra
parameter to _res_send_idx_msg(): for example _res_send_idx_msg(..., no_error_print);
and provide this "no_error_print" to rd_recv_msg(), through "void
*data".

> >
> >
> >         static inline int _##name(struct rd *rd)
> > \
> >         {
> > \
> >                 uint32_t idx;
> > \
> >                 int ret;
> > \
> >                 if (id) {
> > \
> >                         ret = rd_doit_index(rd, &idx);
> > \
> >                         if (ret) {
> > \
> >                                 ret = _res_send_idx_msg(rd, command,
> > \
> >                                                         name##_idx_parse_cb,
> > \
> >                                                         idx, id);
> > \
> >                                 if (!ret)
> > \
> >                                         return ret;
> > \
> >                                 /* Fallback for old systems without .doit
> > callbacks */ \
> >                         }
> > \
> >                 }
> > \
> >                 return _res_send_msg(rd, command, name##_parse_cb);
> > \
> >         }
> > \
> >
> >
> >
> > The "no such file or dir" errors are being returned because, in my setup,
> > there are 2 other links that do not have lqpn 176.   So there are 2 issues
> > uncovered by adding generic printing of errors in rd_recv_msg()
> >
> > 1) the doit code in rdmatool is generating requests for a doit method in the
> > kernel w/o providing a port index.
> > 2) some paths in rdmatool should not print "benign" errors like filtering on
> > a GET command causing a "does not exist" error returned by the kernel doit
> > func.
> >
> > #1 is a bug, IMO.  Can you propose a fix?
> > #2 could be solved by adding an error callback func passed to rd_recv_msg().
> > Then the RES_FUNC() functions could parse errors like "no such file or dir"
> > when doing a filtered query and silently drop them.  And functions like
> > dev_set_name() would display all errors returned because there are no
> > expected errors other than "success".
> >
> > Steve.
> >
>
> Hey Leon, you've been quiet. :)   Thoughts?

I missed your email, sorry.

>
> Thanks,
>
> Steve.
>
>

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