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

2018-09-13 17:19 UTC #1 lore raw

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

In reply to 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 +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

Steve Wise

author

2018-11-28 15:16 UTC #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

Steve Wise

author

2018-11-28 16:25 UTC #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

Steve Wise

author

2018-11-28 19:34 UTC #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).

Leon Romanovsky

2018-11-28 20:02 UTC #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

Leon Romanovsky

2018-11-28 20:04 UTC #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

Steve Wise

author

2018-11-28 20:07 UTC #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...

Leon Romanovsky

2018-11-28 20:08 UTC #6 lore raw

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

In reply to Leon Romanovsky

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

Leon Romanovsky

2018-11-28 20:13 UTC #10 lore raw

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

In reply to Steve Wise

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

Steve Wise

author

2018-11-28 20:18 UTC #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.

Steve Wise

author

2018-11-28 20:23 UTC #7 lore raw

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

In reply to Leon Romanovsky

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...

Jason Gunthorpe

2018-11-28 22:17 UTC #12 lore raw

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

In reply to Steve Wise

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

Steve Wise

author

2018-11-28 22:21 UTC #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.

Jason Gunthorpe

2018-11-28 22:25 UTC #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

Steve Wise

author

2018-11-28 22:51 UTC #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!