| Message ID | 7026be07534b14fd74e592c315523c57fde05a0a.1543422310.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Dynamic rdma link creation | expand |
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 >
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 >>
... >>> + 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).
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 >
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
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...
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 > > >
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 > >
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.
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...
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
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.
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
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!
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;
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(-)