Skip to contentSkip to conversation

Dynamic rdma link creation

RFC PATCH In review Steve Wise 3 participants 17 messages Sep 13, 2018 – Nov 28, 2018

Subsystems (3)
Networking Network drivers InfiniBand
lore
Share & tools
lore

Steve Wise

author
#0 lore raw

cover Dynamic rdma link creation

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.

I've tagged this as "RFC" to start reviewing it, and because it is dependent on kernel changes which are still under review [1].

Please comment!

Thanks,

Steve.

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

Steve Wise (2):
rdma: add 'link add/delete' commands
rdma: man page update for link add/delete

 man/man8/rdma-link.8 |  51 +++++++++++++++++++++++++
 rdma/link.c          | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h          |   1 +
 rdma/utils.c         |   2 +-
 4 files changed, 159 insertions(+), 1 deletion(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

RFC PATCH 1/2 rdma: add 'link add/delete' commands

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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
rdma/link.c +106 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);
 	return 0;
 }
 

@@ -315,10 +317,114 @@ static int link_show(struct rd *rd)
 	return rd_exec_link(rd, link_one_show, true);
 }
 
+static int link_add_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_add(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	char *dev = NULL;
+	uint32_t seq;
+	int ret;
+
+	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, "dev")) {
+			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);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);
+	return ret;
+}
+
+static int link_del_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_del(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	uint32_t seq;
+	int ret;
+
+	if (rd_no_arg(rd)) {
+		pr_err("No link type 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 {
+			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;
+	}
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &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);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_del_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);
+	return ret;
+}
+
 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 },
rdma/rdma.h +1 -0
index 547bb5749a39..b5271e423c10 100644
@@ -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);
rdma/utils.c +1 -1
index 61f4aeb1bcf2..41f8f7391279 100644
@@ -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;
Signature · 1 line
1.8.3.1
#2 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 20 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
rdma/link.c +0 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644

rdma/link.c line 17

Quoted 4 lines
rdma/link.c +1 -0
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Quoted 50 lines
rdma/link.c +43 -0
 	return 0;
 }
 

@@ -315,10 +317,114 @@ static int link_show(struct rd *rd)
 	return rd_exec_link(rd, link_one_show, true);
 }
 
+static int link_add_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_add(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	char *dev = NULL;
+	uint32_t seq;
+	int ret;
+
+	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, "dev")) {
+			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));

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 10 lines
rdma/link.c +10 -0
+	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);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

Quoted 37 lines
rdma/link.c +37 -0
+	return ret;
+}
+
+static int link_del_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_del(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	uint32_t seq;
+	int ret;
+
+	if (rd_no_arg(rd)) {
+		pr_err("No link type 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 {
+			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;
+	}
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
+		       (NLM_F_REQUEST | NLM_F_ACK));

rdma/link.c static int link_show(struct rd *rd) · line 419

Quoted 10 lines
rdma/link.c +10 -0
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_del_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);
+	return ret;

The same question as above.

Quoted 39 lines
rdma/link.c +4 -0
+}
+
 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 },
rdma/rdma.h +1 -0
index 547bb5749a39..b5271e423c10 100644
@@ -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);
rdma/utils.c +1 -2
index 61f4aeb1bcf2..41f8f7391279 100644
@@ -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

author
#3 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 16 lines · Leon Romanovsky
Quoted 15 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 11 lines

rdma/link.c line 17

Quoted 9 lines
rdma/link.c +1 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

Quoted 55 lines
Quoted 55 lines
rdma/link.c +48 -0
 	return 0;
 }
 

@@ -315,10 +317,114 @@ static int link_show(struct rd *rd)
 	return rd_exec_link(rd, link_one_show, true);
 }
 
+static int link_add_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_add(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	char *dev = NULL;
+	uint32_t seq;
+	int ret;
+
+	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, "dev")) {
+			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);
+	ret = rd_send_msg(rd);
+	if (ret)
Quoted 8 lines

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 5 lines
rdma/link.c +5 -0
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

To get the response of if it was successfully added.  It provides the errno value.

Quoted 88 lines
Quoted 37 lines
rdma/link.c +37 -0
+	return ret;
+}
+
+static int link_del_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	return MNL_CB_OK;
+}
+
+static int link_del(struct rd *rd)
+{
+	char *name;
+	char *type = NULL;
+	uint32_t seq;
+	int ret;
+
+	if (rd_no_arg(rd)) {
+		pr_err("No link type 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 {
+			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;
+	}
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
+		       (NLM_F_REQUEST | NLM_F_ACK));

rdma/link.c static int link_show(struct rd *rd) · line 419

Quoted 10 lines
rdma/link.c +10 -0
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
+	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_del_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);
+	return ret;

The same question as above.

Quoted 39 lines
rdma/link.c +4 -0
+}
+
 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 },
rdma/rdma.h +1 -0
index 547bb5749a39..b5271e423c10 100644
@@ -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);
rdma/utils.c +1 -2
index 61f4aeb1bcf2..41f8f7391279 100644
@@ -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

author
#4 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

...

Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
rdma/link.c +7 -0
+	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);
+	ret = rd_send_msg(rd);
+	if (ret)
Quoted 8 lines

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 5 lines
rdma/link.c +5 -0
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

Quoted 2 lines

To get the response of if it was successfully added.  It provides the errno value.

If I don't do the rd_recv_msg, then adding the same name twice fails without any error notification.  Ditto for deleting a non-existent link.  So the rd_recv_msg() allows getting the failure reason (and detecting the failure).

#5 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 17 lines · Steve Wise

...

Quoted 15 lines
Quoted 15 lines

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 12 lines
rdma/link.c +12 -0
+	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);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

Quoted 7 lines
Quoted 2 lines

To get the response of if it was successfully added.  It provides the errno value.

If I don't do the rd_recv_msg, then adding the same name twice fails without any error notification.  Ditto for deleting a non-existent link.  So the rd_recv_msg() allows getting the failure reason (and detecting the failure).

Shouldn't extack provide such information as part of NLM_F_ACK flag?

just shooting into the air, will take more close look tomorrow.

Thanks

Quoted 1 line
#6 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 26 lines · Leon Romanovsky
Quoted 17 lines · Steve Wise

...

Quoted 15 lines
Quoted 15 lines

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 12 lines
rdma/link.c +12 -0
+	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);
+	ret = rd_send_msg(rd);
+	if (ret)
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

Quoted 7 lines
Quoted 2 lines

To get the response of if it was successfully added.  It provides the errno value.

If I don't do the rd_recv_msg, then adding the same name twice fails without any error notification.  Ditto for deleting a non-existent link.  So the rd_recv_msg() allows getting the failure reason (and detecting the failure).

Quoted 3 lines

Shouldn't extack provide such information as part of NLM_F_ACK flag?

just shooting into the air, will take more close look tomorrow.

OK, it was easier than I thought.

You are right, need both send and receive to get the reason.

Can you prepare general function and update rename part too? Something like send_receive(...) with dummy callback for receive path.

Thanks

Quoted 4 lines

Thanks

Quoted 1 line
author
#7 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 29 lines · Leon Romanovsky
Quoted 20 lines · Leon Romanovsky
Quoted 19 lines · Steve Wise

...

Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
rdma/link.c +7 -0
+	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);
+	ret = rd_send_msg(rd);
+	if (ret)
Quoted 8 lines

rdma/link.c static int link_show(struct rd *rd) · line 372

Quoted 5 lines
rdma/link.c +5 -0
+		return ret;
+
+	ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
+	if (ret)
+		perror(NULL);

Why do you need rd_recv_msg()? I think that it is not needed, at least for rename, I didn't need it.
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244

To get the response of if it was successfully added.  It provides the errno value.

Quoted 8 lines
Quoted 5 lines

If I don't do the rd_recv_msg, then adding the same name twice fails without any error notification.  Ditto for deleting a non-existent link.  So the rd_recv_msg() allows getting the failure reason (and detecting the failure).

Shouldn't extack provide such information as part of NLM_F_ACK flag?

just shooting into the air, will take more close look tomorrow.

Quoted 8 lines

OK, it was easier than I thought.

You are right, need both send and receive to get the reason.

Can you prepare general function and update rename part too? Something like send_receive(...) with dummy callback for receive path.

Thanks

Sure, I'll whip something up for the next version of the patch series...

#8 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

In reply to Steve Wise
Quoted 31 lines · Steve Wise
Quoted 16 lines · Leon Romanovsky
Quoted 15 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 11 lines

rdma/link.c line 17

Quoted 9 lines
rdma/link.c +1 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Quoted 8 lines

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

I would say, yes.
It makes "delete" operation more natural.

Thanks

author
#9 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 34 lines · Leon Romanovsky
Quoted 33 lines · Steve Wise
Quoted 16 lines · Leon Romanovsky
Quoted 15 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 11 lines

rdma/link.c line 17

Quoted 9 lines
rdma/link.c +1 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Quoted 7 lines
Quoted 3 lines

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

I would say, yes.
It makes "delete" operation more natural.

Thanks

Perhaps.

Note: 'ip link delete' takes a type as well...

#10 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 40 lines · Steve Wise
Quoted 37 lines · Leon Romanovsky
Quoted 36 lines · Steve Wise
Quoted 16 lines · Leon Romanovsky
Quoted 15 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 11 lines

rdma/link.c line 17

Quoted 9 lines
rdma/link.c +1 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

Quoted 8 lines
Quoted 4 lines

I would say, yes.
It makes "delete" operation more natural.

Thanks

Perhaps.

Note: 'ip link delete' takes a type as well...

According to man section, yes.
According to various guides, no.
https://docs.fedoraproject.org/en-US/Fedora/20/html/Networking_Guide/sec-Configure_802_1Q_VLAN_Tagging_ip_Commands.html

Thanks

Quoted 2 lines
author
#11 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 43 lines · Leon Romanovsky
Quoted 42 lines · Steve Wise
Quoted 40 lines · Leon Romanovsky
Quoted 28 lines · Steve Wise
Quoted 16 lines · Leon Romanovsky
Quoted 15 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 11 lines

rdma/link.c line 17

Quoted 9 lines
rdma/link.c +1 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Quoted 7 lines

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

I would say, yes.
It makes "delete" operation more natural.

Thanks

Quoted 8 lines
Quoted 3 lines

Perhaps.

Note: 'ip link delete' takes a type as well...

According to man section, yes.
According to various guides, no.
https://docs.fedoraproject.org/en-US/Fedora/20/html/Networking_Guide/sec-Configure_802_1Q_VLAN_Tagging_ip_Commands.html

Thanks

It does make sense to not require type.  The name must be unique so that should be enough.  I'll have to respin the kernel side though...

Thanks,

Steve.

#12 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 47 lines · Steve Wise
Quoted 44 lines · Leon Romanovsky
Quoted 43 lines · Steve Wise
Quoted 31 lines · Leon Romanovsky
Quoted 30 lines · Steve Wise
Quoted 15 lines · Leon Romanovsky
Quoted 14 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 10 lines

rdma/link.c line 17

Quoted 8 lines
rdma/link.c +2 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
+++ b/rdma/link.c

@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Quoted 7 lines
Quoted 3 lines

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

I would say, yes.
It makes "delete" operation more natural.

Thanks

Perhaps.

Note: 'ip link delete' takes a type as well...

Quoted 8 lines
Quoted 5 lines

According to man section, yes.
According to various guides, no.
https://docs.fedoraproject.org/en-US/Fedora/20/html/Networking_Guide/sec-Configure_802_1Q_VLAN_Tagging_ip_Commands.html

Thanks

It does make sense to not require type.  The name must be unique so that should be enough.  I'll have to respin the kernel side though...

The delete_link really should be an operation on the ib_device, not the link_ops thing.

That directly prevents mis-matching function callbacks..

Jason

author
#13 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 51 lines · Jason Gunthorpe
Quoted 50 lines · Steve Wise
Quoted 48 lines · Leon Romanovsky
Quoted 35 lines · Steve Wise
Quoted 34 lines · Leon Romanovsky
Quoted 33 lines · Steve Wise
Quoted 15 lines · Leon Romanovsky
Quoted 14 lines · 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 dev eth0
rdma link delete rxe_eth0

Signed-off-by Steve Wise <[email protected]>
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)
Quoted 10 lines

rdma/link.c line 17

Quoted 8 lines
rdma/link.c +2 -0
index 7a6d4b7e356d..d4f76b0ce11f 100644
+++ b/rdma/link.c

@@ -14,6 +14,8 @@
 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 dev DEV\n", rd->filename);

I suggest to rename "dev" to be "netdev", because we are using "dev" for ib devices.

Yea ok.

Quoted 2 lines

rdma/link.c line 18

Quoted 1 line
rdma/link.c +1 -0
+	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

Why do you need "type" for "delete" command?

Because the type is used in the kernel to find the appropriate link ops.  I could change the kernel side to search all types for the device name to delete?

Quoted 7 lines
Quoted 4 lines

I would say, yes.
It makes "delete" operation more natural.

Thanks

Perhaps.

Note: 'ip link delete' takes a type as well...

According to man section, yes.
According to various guides, no.
https://docs.fedoraproject.org/en-US/Fedora/20/html/Networking_Guide/sec-Configure_802_1Q_VLAN_Tagging_ip_Commands.html

Thanks

Quoted 8 lines
Quoted 2 lines

It does make sense to not require type.  The name must be unique so that should be enough.  I'll have to respin the kernel side though...

The delete_link really should be an operation on the ib_device, not the link_ops thing.

That directly prevents mis-matching function callbacks..

Jason

Looking at the rtnetlink newlink/dellink, I see they cache the link_ops ptr in the net_device struct.  So when the link is deleted, then appropriate driver-specific dellink function can be called after finding the device to be deleted.  Should I do something along these lines?  IE add a struct rdma_link_ops pointer to struct ib_device.

Steve.

#14 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 5 lines · Steve Wise
Quoted 5 lines
Quoted 2 lines

It does make sense to not require type.  The name must be unique so that should be enough.  I'll have to respin the kernel side though...

The delete_link really should be an operation on the ib_device, not the link_ops thing.

Quoted 8 lines
Quoted 3 lines

That directly prevents mis-matching function callbacks..

Jason

Looking at the rtnetlink newlink/dellink, I see they cache the link_ops ptr in the net_device struct.  So when the link is deleted, then appropriate driver-specific dellink function can be called after finding the device to be deleted.  Should I do something along these lines?  IE add a struct rdma_link_ops pointer to struct ib_device.

I don't see a problem with that either..

Jason

author
#15 lore raw

Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands

Quoted 10 lines · Jason Gunthorpe
Quoted 8 lines · Steve Wise
Quoted 8 lines
Quoted 2 lines

It does make sense to not require type.  The name must be unique so that should be enough.  I'll have to respin the kernel side though...

The delete_link really should be an operation on the ib_device, not the link_ops thing.

That directly prevents mis-matching function callbacks..

Jason

Quoted 8 lines
Quoted 5 lines

Looking at the rtnetlink newlink/dellink, I see they cache the link_ops ptr in the net_device struct.  So when the link is deleted, then appropriate driver-specific dellink function can be called after finding the device to be deleted.  Should I do something along these lines?  IE add a struct rdma_link_ops pointer to struct ib_device.

I don't see a problem with that either..

Jason

Ok, I'll respin the kernel and user patches tomorrow.  Thanks!

author
#16 lore raw

RFC PATCH 2/2 rdma: man page update for link add/delete

In reply to Steve Wise

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

Signed-off-by Steve Wise <[email protected]>
---
 man/man8/rdma-link.8 | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
man/man8/rdma-link.8 +51 -0
index 97dd8bb994d2..b994c326dc34 100644
@@ -23,6 +23,20 @@ rdma-link \- rdma link configuration
 .RI "[ " DEV/PORT_INDEX " ]"
 
 .ti -8
+.B rdma link add
+.BR NAME
+.BR type
+.BR TYPE
+.BR dev
+.BR NETDEV
+
+.ti -8
+.B rdma link delete
+.RI NAME
+.BR type
+.BR TYPE
+
+.ti -8
 .B rdma link help
 
 .SH "DESCRIPTION"

@@ -33,6 +47,33 @@ 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 dev 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 type TYPE - delete an rdma link for the specified type
+.PP
+.BR NAME
+- specifies the name of the rdma link to delete
+.PP
+.BR TYPE
+- specifies which rdma type to use
+
 .SH "EXAMPLES"
 .PP
 rdma link show

@@ -45,6 +86,16 @@ rdma link show mlx5_2/1
 Shows the state of specified rdma link.
 .RE
 .PP
+rdma link add siw_eth0 type rxe dev eth0
+.RS 4
+Adds a RXE link named siw_eth0 to network device eth0
+.RE
+.PP
+rdma link del siw_eth0 type rxe
+.RS 4
+Removes RXE link siw_eth0
+.RE
+.PP
 
 .SH SEE ALSO
 .BR rdma (8),
Signature · 1 line
1.8.3.1