All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 16:25 ` Steve Wise
@ 2018-09-13 17:19   ` Steve Wise
  -1 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-09-13 17:19 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, linux-rdma, leon, BMT

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 <swise@opengridcomputing.com>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/rdma/link.c b/rdma/link.c
index 7a6d4b7e356d..d4f76b0ce11f 100644
--- a/rdma/link.c
+++ 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);
+	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 },
diff --git a/rdma/rdma.h b/rdma/rdma.h
index 547bb5749a39..b5271e423c10 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 61f4aeb1bcf2..41f8f7391279 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
@ 2018-09-13 17:19   ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-09-13 17:19 UTC (permalink / raw)
  To: dsahern, leon; +Cc: stephen, netdev, linux-rdma, leon, BMT

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 <swise@opengridcomputing.com>
---
 rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rdma/rdma.h  |   1 +
 rdma/utils.c |   2 +-
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/rdma/link.c b/rdma/link.c
index 7a6d4b7e356d..d4f76b0ce11f 100644
--- a/rdma/link.c
+++ 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);
+	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 },
diff --git a/rdma/rdma.h b/rdma/rdma.h
index 547bb5749a39..b5271e423c10 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 61f4aeb1bcf2..41f8f7391279 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC iproute2-next 2/2] rdma: man page update for link add/delete
  2018-11-28 16:25 ` Steve Wise
@ 2018-11-28 15:16   ` Steve Wise
  -1 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 15:16 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, linux-rdma, leon, BMT

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

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

diff --git a/man/man8/rdma-link.8 b/man/man8/rdma-link.8
index 97dd8bb994d2..b994c326dc34 100644
--- a/man/man8/rdma-link.8
+++ b/man/man8/rdma-link.8
@@ -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),
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC iproute2-next 2/2] rdma: man page update for link add/delete
@ 2018-11-28 15:16   ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 15:16 UTC (permalink / raw)
  To: dsahern, leon; +Cc: stephen, netdev, linux-rdma, leon, BMT

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

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

diff --git a/man/man8/rdma-link.8 b/man/man8/rdma-link.8
index 97dd8bb994d2..b994c326dc34 100644
--- a/man/man8/rdma-link.8
+++ b/man/man8/rdma-link.8
@@ -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),
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH RFC iproute2-next 0/2] Dynamic rdma link creation
@ 2018-11-28 16:25 ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 16:25 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, linux-rdma, leon, BMT

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(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH RFC iproute2-next 0/2] Dynamic rdma link creation
@ 2018-11-28 16:25 ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 16:25 UTC (permalink / raw)
  To: dsahern, leon; +Cc: stephen, netdev, linux-rdma, leon, BMT

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(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-09-13 17:19   ` Steve Wise
  (?)
@ 2018-11-28 18:26   ` Leon Romanovsky
  2018-11-28 19:08     ` Steve Wise
  -1 siblings, 1 reply; 20+ messages in thread
From: Leon Romanovsky @ 2018-11-28 18:26 UTC (permalink / raw)
  To: Steve Wise; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

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

On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
> rdma link delete rxe_eth0
>
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rdma/rdma.h  |   1 +
>  rdma/utils.c |   2 +-
>  3 files changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/rdma/link.c b/rdma/link.c
> index 7a6d4b7e356d..d4f76b0ce11f 100644
> --- a/rdma/link.c
> +++ 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.

> +	pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);

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

>  	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);

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

> +	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;

The same question as above.

> +}
> +
>  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 547bb5749a39..b5271e423c10 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 61f4aeb1bcf2..41f8f7391279 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 --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 18:26   ` Leon Romanovsky
@ 2018-11-28 19:08     ` Steve Wise
  2018-11-28 19:34       ` Steve Wise
  2018-11-28 20:04       ` Leon Romanovsky
  0 siblings, 2 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 19:08 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
>> rdma link delete rxe_eth0
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  rdma/rdma.h  |   1 +
>>  rdma/utils.c |   2 +-
>>  3 files changed, 108 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/link.c b/rdma/link.c
>> index 7a6d4b7e356d..d4f76b0ce11f 100644
>> --- a/rdma/link.c
>> +++ 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.

>> +	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? 

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

>> +	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;
> The same question as above.
>
>> +}
>> +
>>  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 547bb5749a39..b5271e423c10 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 61f4aeb1bcf2..41f8f7391279 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
>>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 19:08     ` Steve Wise
@ 2018-11-28 19:34       ` Steve Wise
  2018-11-28 20:02         ` Leon Romanovsky
  2018-11-28 20:04       ` Leon Romanovsky
  1 sibling, 1 reply; 20+ messages in thread
From: Steve Wise @ 2018-11-28 19:34 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

...

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 19:34       ` Steve Wise
@ 2018-11-28 20:02         ` Leon Romanovsky
  2018-11-28 20:08           ` Leon Romanovsky
  0 siblings, 1 reply; 20+ messages in thread
From: Leon Romanovsky @ 2018-11-28 20:02 UTC (permalink / raw)
  To: Steve Wise; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

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

On Wed, Nov 28, 2018 at 01:34:14PM -0600, Steve Wise wrote:
> ...
>
> >>> +	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
> > 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

>

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 19:08     ` Steve Wise
  2018-11-28 19:34       ` Steve Wise
@ 2018-11-28 20:04       ` Leon Romanovsky
  2018-11-28 20:07         ` Steve Wise
  1 sibling, 1 reply; 20+ messages in thread
From: Leon Romanovsky @ 2018-11-28 20:04 UTC (permalink / raw)
  To: Steve Wise; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

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

On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
>
>
> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
> > On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
> >> rdma link delete rxe_eth0
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  rdma/rdma.h  |   1 +
> >>  rdma/utils.c |   2 +-
> >>  3 files changed, 108 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/link.c b/rdma/link.c
> >> index 7a6d4b7e356d..d4f76b0ce11f 100644
> >> --- a/rdma/link.c
> >> +++ 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.
>
> >> +	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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:04       ` Leon Romanovsky
@ 2018-11-28 20:07         ` Steve Wise
  2018-11-28 20:13           ` Leon Romanovsky
  0 siblings, 1 reply; 20+ messages in thread
From: Steve Wise @ 2018-11-28 20:07 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 2:04 PM, Leon Romanovsky wrote:
> On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
>>
>> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
>>> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
>>>> rdma link delete rxe_eth0
>>>>
>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>>> ---
>>>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  rdma/rdma.h  |   1 +
>>>>  rdma/utils.c |   2 +-
>>>>  3 files changed, 108 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/rdma/link.c b/rdma/link.c
>>>> index 7a6d4b7e356d..d4f76b0ce11f 100644
>>>> --- a/rdma/link.c
>>>> +++ 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.
>>
>>>> +	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

Perhaps.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:02         ` Leon Romanovsky
@ 2018-11-28 20:08           ` Leon Romanovsky
  2018-11-28 20:23             ` Steve Wise
  0 siblings, 1 reply; 20+ messages in thread
From: Leon Romanovsky @ 2018-11-28 20:08 UTC (permalink / raw)
  To: Steve Wise; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

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

On Wed, Nov 28, 2018 at 10:02:04PM +0200, Leon Romanovsky wrote:
> On Wed, Nov 28, 2018 at 01:34:14PM -0600, Steve Wise wrote:
> > ...
> >
> > >>> +	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
> > > 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.

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

>
> Thanks
>
> >



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:07         ` Steve Wise
@ 2018-11-28 20:13           ` Leon Romanovsky
  2018-11-28 20:18             ` Steve Wise
  0 siblings, 1 reply; 20+ messages in thread
From: Leon Romanovsky @ 2018-11-28 20:13 UTC (permalink / raw)
  To: Steve Wise; +Cc: dsahern, stephen, netdev, linux-rdma, BMT

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

On Wed, Nov 28, 2018 at 02:07:29PM -0600, Steve Wise wrote:
>
>
> On 11/28/2018 2:04 PM, Leon Romanovsky wrote:
> > On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
> >>
> >> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
> >>> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
> >>>> rdma link delete rxe_eth0
> >>>>
> >>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >>>> ---
> >>>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  rdma/rdma.h  |   1 +
> >>>>  rdma/utils.c |   2 +-
> >>>>  3 files changed, 108 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/rdma/link.c b/rdma/link.c
> >>>> index 7a6d4b7e356d..d4f76b0ce11f 100644
> >>>> --- a/rdma/link.c
> >>>> +++ 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.
> >>
> >>>> +	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
>
> 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

>
>

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:13           ` Leon Romanovsky
@ 2018-11-28 20:18             ` Steve Wise
  2018-11-28 22:17               ` Jason Gunthorpe
  0 siblings, 1 reply; 20+ messages in thread
From: Steve Wise @ 2018-11-28 20:18 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 2:13 PM, Leon Romanovsky wrote:
> On Wed, Nov 28, 2018 at 02:07:29PM -0600, Steve Wise wrote:
>>
>> On 11/28/2018 2:04 PM, Leon Romanovsky wrote:
>>> On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
>>>> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
>>>>> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
>>>>>> rdma link delete rxe_eth0
>>>>>>
>>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>>>>> ---
>>>>>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  rdma/rdma.h  |   1 +
>>>>>>  rdma/utils.c |   2 +-
>>>>>>  3 files changed, 108 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/rdma/link.c b/rdma/link.c
>>>>>> index 7a6d4b7e356d..d4f76b0ce11f 100644
>>>>>> --- a/rdma/link.c
>>>>>> +++ 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.
>>>>
>>>>>> +	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
>> 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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:08           ` Leon Romanovsky
@ 2018-11-28 20:23             ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 20:23 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 2:08 PM, Leon Romanovsky wrote:
> On Wed, Nov 28, 2018 at 10:02:04PM +0200, Leon Romanovsky wrote:
>> On Wed, Nov 28, 2018 at 01:34:14PM -0600, Steve Wise wrote:
>>> ...
>>>
>>>>>> +	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
>>>> 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.
> 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...

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 20:18             ` Steve Wise
@ 2018-11-28 22:17               ` Jason Gunthorpe
  2018-11-28 22:21                 ` Steve Wise
  0 siblings, 1 reply; 20+ messages in thread
From: Jason Gunthorpe @ 2018-11-28 22:17 UTC (permalink / raw)
  To: Steve Wise; +Cc: Leon Romanovsky, dsahern, stephen, netdev, linux-rdma, BMT

On Wed, Nov 28, 2018 at 02:18:55PM -0600, Steve Wise wrote:
> 
> 
> On 11/28/2018 2:13 PM, Leon Romanovsky wrote:
> > On Wed, Nov 28, 2018 at 02:07:29PM -0600, Steve Wise wrote:
> >>
> >> On 11/28/2018 2:04 PM, Leon Romanovsky wrote:
> >>> On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
> >>>> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
> >>>>> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
> >>>>>> rdma link delete rxe_eth0
> >>>>>>
> >>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >>>>>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  rdma/rdma.h  |   1 +
> >>>>>>  rdma/utils.c |   2 +-
> >>>>>>  3 files changed, 108 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/rdma/link.c b/rdma/link.c
> >>>>>> 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.
> >>>>
> >>>>>> +	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
> >> 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...

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 22:17               ` Jason Gunthorpe
@ 2018-11-28 22:21                 ` Steve Wise
  2018-11-28 22:25                   ` Jason Gunthorpe
  0 siblings, 1 reply; 20+ messages in thread
From: Steve Wise @ 2018-11-28 22:21 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 4:17 PM, Jason Gunthorpe wrote:
> On Wed, Nov 28, 2018 at 02:18:55PM -0600, Steve Wise wrote:
>>
>> On 11/28/2018 2:13 PM, Leon Romanovsky wrote:
>>> On Wed, Nov 28, 2018 at 02:07:29PM -0600, Steve Wise wrote:
>>>> On 11/28/2018 2:04 PM, Leon Romanovsky wrote:
>>>>> On Wed, Nov 28, 2018 at 01:08:05PM -0600, Steve Wise wrote:
>>>>>> On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
>>>>>>> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
>>>>>>>> rdma link delete rxe_eth0
>>>>>>>>
>>>>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>>>>>>>  rdma/link.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  rdma/rdma.h  |   1 +
>>>>>>>>  rdma/utils.c |   2 +-
>>>>>>>>  3 files changed, 108 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/rdma/link.c b/rdma/link.c
>>>>>>>> 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.
>>>>>>
>>>>>>>> +	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
>>>> 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...
> 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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 22:21                 ` Steve Wise
@ 2018-11-28 22:25                   ` Jason Gunthorpe
  2018-11-28 22:51                     ` Steve Wise
  0 siblings, 1 reply; 20+ messages in thread
From: Jason Gunthorpe @ 2018-11-28 22:25 UTC (permalink / raw)
  To: Steve Wise; +Cc: Leon Romanovsky, dsahern, stephen, netdev, linux-rdma, BMT

On Wed, Nov 28, 2018 at 04:21:48PM -0600, Steve Wise wrote:

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

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

Jason

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands
  2018-11-28 22:25                   ` Jason Gunthorpe
@ 2018-11-28 22:51                     ` Steve Wise
  0 siblings, 0 replies; 20+ messages in thread
From: Steve Wise @ 2018-11-28 22:51 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, dsahern, stephen, netdev, linux-rdma, BMT



On 11/28/2018 4:25 PM, Jason Gunthorpe wrote:
> On Wed, Nov 28, 2018 at 04:21:48PM -0600, Steve Wise wrote:
>
>>>> 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.
> I don't see a problem with that either..
>
> Jason

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-11-29  3:29 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-28 16:25 [PATCH RFC iproute2-next 0/2] Dynamic rdma link creation Steve Wise
2018-11-28 16:25 ` Steve Wise
2018-09-13 17:19 ` [PATCH RFC iproute2-next 1/2] rdma: add 'link add/delete' commands Steve Wise
2018-09-13 17:19   ` Steve Wise
2018-11-28 18:26   ` Leon Romanovsky
2018-11-28 19:08     ` Steve Wise
2018-11-28 19:34       ` Steve Wise
2018-11-28 20:02         ` Leon Romanovsky
2018-11-28 20:08           ` Leon Romanovsky
2018-11-28 20:23             ` Steve Wise
2018-11-28 20:04       ` Leon Romanovsky
2018-11-28 20:07         ` Steve Wise
2018-11-28 20:13           ` Leon Romanovsky
2018-11-28 20:18             ` Steve Wise
2018-11-28 22:17               ` Jason Gunthorpe
2018-11-28 22:21                 ` Steve Wise
2018-11-28 22:25                   ` Jason Gunthorpe
2018-11-28 22:51                     ` Steve Wise
2018-11-28 15:16 ` [PATCH RFC iproute2-next 2/2] rdma: man page update for link add/delete Steve Wise
2018-11-28 15:16   ` Steve Wise

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.