diff mbox series

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

Message ID 5dd9d9aeada48bc5db0eb0394ed4e3ce38ee41bc.1550773362.git.swise@opengridcomputing.com (mailing list archive)
State Not Applicable
Headers show
Series Dynamic rdma link creation | expand

Commit Message

Steve Wise Feb. 21, 2019, 4:19 p.m. UTC 
Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
device to a netdev interface.

EG:

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

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

Comments

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

This is great that we finally got here!

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

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

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

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

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

General parser handle it.

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

rd_exec_require_dev() ???

> +
> +	rd_prepare_msg(rd, RDMA_NLDEV_CMD_NEWLINK, &seq,
> +		       (NLM_F_REQUEST | NLM_F_ACK));
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
> +	mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_NDEV_NAME, dev);
> +	return rd_sendrecv_msg(rd, seq);
> +}
> +
> +static int _link_del(struct rd *rd)
> +{
> +	uint32_t seq;
> +
> +	if (!rd_no_arg(rd)) {
> +		pr_err("Unknown parameter %s\n", rd_argv(rd));
> +		return -EINVAL;
> +	}
> +	rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
> +		       (NLM_F_REQUEST | NLM_F_ACK));
> +	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
> +	return rd_sendrecv_msg(rd, seq);
> +}
> +
> +static int link_del(struct rd *rd)
> +{
> +	return rd_exec_require_dev(rd, _link_del);
> +}
> +
>  int cmd_link(struct rd *rd)
>  {
>  	const struct rd_cmd cmds[] = {
>  		{ NULL,		link_show },
> +		{ "add",	link_add },
> +		{ "delete",	link_del },
>  		{ "show",	link_show },
>  		{ "list",	link_show },
>  		{ "help",	link_help },
> diff --git a/rdma/rdma.h b/rdma/rdma.h
> index 20be2f12c4f8..af4597f45640 100644
> --- a/rdma/rdma.h
> +++ b/rdma/rdma.h
> @@ -79,6 +79,7 @@ struct rd_cmd {
>   */
>  bool rd_no_arg(struct rd *rd);
>  void rd_arg_inc(struct rd *rd);
> +bool rd_argv_match(struct rd *rd, const char *pattern);
>
>  char *rd_argv(struct rd *rd);
>
> diff --git a/rdma/utils.c b/rdma/utils.c
> index a6f2826c9605..85a954167511 100644
> --- a/rdma/utils.c
> +++ b/rdma/utils.c
> @@ -32,7 +32,7 @@ int strcmpx(const char *str1, const char *str2)
>  	return strncmp(str1, str2, strlen(str1));
>  }
>
> -static bool rd_argv_match(struct rd *rd, const char *pattern)
> +bool rd_argv_match(struct rd *rd, const char *pattern)
>  {
>  	if (!rd_argc(rd))
>  		return false;
> --
> 1.8.3.1
>
Steve Wise Feb. 26, 2019, 5:24 p.m. UTC | #3 
On 2/23/2019 3:43 AM, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
>> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-rdma
>> device to a netdev interface.
>>
>> EG:
>>
>> rdma link add rxe_eth0 type rxe netdev eth0
>> rdma link delete rxe_eth0
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>>  rdma/link.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  rdma/rdma.h  |  1 +
>>  rdma/utils.c |  2 +-
>>  3 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/link.c b/rdma/link.c
>> index c064be627be2..afaf19663728 100644
>> --- a/rdma/link.c
>> +++ b/rdma/link.c
>> @@ -14,6 +14,9 @@
>>  static int link_help(struct rd *rd)
>>  {
>>  	pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
>> +	pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
>> +	       rd->filename);
>> +	pr_out("Usage: %s link delete NAME\n", rd->filename);
>>  	return 0;
>>  }
>>
>> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
>>  	return rd_exec_link(rd, link_one_show, true);
>>  }
>>
>> +static int link_add(struct rd *rd)
>> +{
>> +	char *name;
>> +	char *type = NULL;
>> +	char *dev = NULL;
>> +	uint32_t seq;
>> +
>> +	if (rd_no_arg(rd)) {
>> +		pr_err("No link name was supplied\n");
> I think that it is better to have instruction message and not error
> message: "Please provide ...".


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


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


Ok.  Like your recently merged series did...


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


Ok.


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


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


Thanks!


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

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

Steve
diff mbox series

Patch

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