| Message ID | 1458139222-4863-1-git-send-email-mauricio.vasquezbernal@studenti.polito.it |
|---|---|
| State | Changes Requested |
| Headers | show |
Somebody wants to have at look at this? Thanks, On Wed, Mar 16, 2016 at 3:40 PM, Mauricio Vasquez B < mauricio.vasquezbernal@studenti.polito.it> wrote: > In order to use dpdk ports in ovs they have to be bound to a DPDK > compatible driver before ovs is started. > > This patch adds the possibility to hotplug (or hot-unplug) a device > after ovs has been started. The implementation adds two appctl commands: > netdev-dpdk/attach and netdev-dpdk/detach. > > After the user attaches a new device, it can use the add-port command > to use it in a switch, similarly, before detaching a device, it has to > be removed using the del-port command. > > Signed-off-by: Mauricio Vasquez B < > mauricio.vasquezbernal@studenti.polito.it> > --- > lib/netdev-dpdk.c | 79 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index f402354..c3a99e7 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void) > } > > static void > +netdev_dpdk_attach(struct unixctl_conn *conn, int argc, > + const char *argv[], void *aux OVS_UNUSED) > +{ > + int ret; > + uint8_t port_id; > + char response[512]; > + > + ovs_mutex_lock(&dpdk_mutex); > + > + ret = rte_eth_dev_attach(argv[1], &port_id); > + if(ret < 0) { > + snprintf(response, sizeof(response), > + "Error attaching device ''", argv[1]); > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + > + snprintf(response, sizeof(response), > + "Device '%s' has been attached as 'dpdk%d'", argv[1], > port_id); > + unixctl_command_reply(conn, response); > + > +unlock: > + ovs_mutex_unlock(&dpdk_mutex); > +} > + > +static void > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc, > + const char *argv[], void *aux OVS_UNUSED) > +{ > + int ret; > + unsigned int port_id; > + char name[RTE_ETH_NAME_MAX_LEN]; > + char response[512]; > + > + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id); > + if(ret) { > + snprintf(response, sizeof(response), > + "'%s' is not a valid dpdk device", argv[1]); > + unixctl_command_reply_error(conn, response); > + return; > + } > + > + ovs_mutex_lock(&dpdk_mutex); > + > + struct netdev * netdev = netdev_from_name(argv[1]); > + if(netdev) { > + netdev_close(netdev); > + snprintf(response, sizeof(response), > + "Port '%s' is being used. Remove it before > detaching", argv[1]); > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + > + rte_eth_dev_close(port_id); > + > + ret = rte_eth_dev_detach(port_id, name); > + if(ret < 0) { > + snprintf(response, sizeof(response), > + "Port '%s' can not be detached", argv[1]); > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + > + snprintf(response, sizeof(response), "Port '%s' has been detached", > argv[1]); > + unixctl_command_reply(conn, response); > + > +unlock: > + ovs_mutex_unlock(&dpdk_mutex); > +} > + > +static void > dpdk_common_init(void) > { > unixctl_command_register("netdev-dpdk/set-admin-state", > "[netdev] up|down", 1, 2, > netdev_dpdk_set_admin_state, NULL); > > + unixctl_command_register("netdev-dpdk/attach", > + "device", 1, 1, > + netdev_dpdk_attach, NULL); > + > + unixctl_command_register("netdev-dpdk/detach", > + "port", 1, 1, > + netdev_dpdk_detach, NULL); > + > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); > } > > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Hi Mauricio, Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> writes: > In order to use dpdk ports in ovs they have to be bound to a DPDK > compatible driver before ovs is started. > > This patch adds the possibility to hotplug (or hot-unplug) a device > after ovs has been started. The implementation adds two appctl commands: > netdev-dpdk/attach and netdev-dpdk/detach. > > After the user attaches a new device, it can use the add-port command > to use it in a switch, similarly, before detaching a device, it has to > be removed using the del-port command. > > Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> > --- I like the patch a lot, it really helps with the ease-of-use of OVS. So, thanks very much for the work. > lib/netdev-dpdk.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index f402354..c3a99e7 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void) > } > > static void > +netdev_dpdk_attach(struct unixctl_conn *conn, int argc, > + const char *argv[], void *aux OVS_UNUSED) ^ I think there's a spacing issue here. > +{ > + int ret; > + uint8_t port_id; > + char response[512]; > + > + ovs_mutex_lock(&dpdk_mutex); > + > + ret = rte_eth_dev_attach(argv[1], &port_id); > + if(ret < 0) { ^ Spacing issue here > + snprintf(response, sizeof(response), > + "Error attaching device ''", argv[1]); ^ Spacing issue here > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + Does it make sense to also create the netdev at this point? > + snprintf(response, sizeof(response), > + "Device '%s' has been attached as 'dpdk%d'", argv[1], port_id); ^ Spacing issue here. > + unixctl_command_reply(conn, response); > + > +unlock: > + ovs_mutex_unlock(&dpdk_mutex); > +} > + > +static void > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc, > + const char *argv[], void *aux OVS_UNUSED) ^ Spacing issue here. > +{ > + int ret; > + unsigned int port_id; > + char name[RTE_ETH_NAME_MAX_LEN]; > + char response[512]; > + > + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id); > + if(ret) { ^ Spacing issue here. > + snprintf(response, sizeof(response), > + "'%s' is not a valid dpdk device", argv[1]); ^ Spacing issue here. > + unixctl_command_reply_error(conn, response); > + return; > + } > + > + ovs_mutex_lock(&dpdk_mutex); > + > + struct netdev * netdev = netdev_from_name(argv[1]); > + if(netdev) { ^ Spacing issue here. > + netdev_close(netdev); > + snprintf(response, sizeof(response), > + "Port '%s' is being used. Remove it before detaching", argv[1]); > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + > + rte_eth_dev_close(port_id); > + > + ret = rte_eth_dev_detach(port_id, name); > + if(ret < 0) { ^ Spacing issue here. > + snprintf(response, sizeof(response), > + "Port '%s' can not be detached", argv[1]); > + unixctl_command_reply_error(conn, response); > + goto unlock; > + } > + > + snprintf(response, sizeof(response), "Port '%s' has been detached", argv[1]); ^ Please stick to 80-bytes on a line. > + unixctl_command_reply(conn, response); > + > +unlock: > + ovs_mutex_unlock(&dpdk_mutex); > +} > + > +static void > dpdk_common_init(void) > { > unixctl_command_register("netdev-dpdk/set-admin-state", > "[netdev] up|down", 1, 2, > netdev_dpdk_set_admin_state, NULL); > > + unixctl_command_register("netdev-dpdk/attach", > + "device", 1, 1, > + netdev_dpdk_attach, NULL); > + > + unixctl_command_register("netdev-dpdk/detach", > + "port", 1, 1, > + netdev_dpdk_detach, NULL); > + Does it make sense to have a single command ``netdev-dpdk/port-ctl'' that takes either attach/detach as an argument? I think it looks a bit better, and you might be able to combine a lot of the code above into a single function, but I have been wrong on user-experience stuff before. > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); > } Thanks again, -Aaron
Hi Aaron, First of all thank you very much for your feedback. I didn't pay that attention to the style, I'll correct all the issues when I send a proper patch, by the way, is there any script or tool to check the style?, I looked for it and I cannot find anything. On Wed, Mar 23, 2016 at 11:42 AM, Aaron Conole <aconole@redhat.com> wrote: > Hi Mauricio, > > Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> writes: > > > In order to use dpdk ports in ovs they have to be bound to a DPDK > > compatible driver before ovs is started. > > > > This patch adds the possibility to hotplug (or hot-unplug) a device > > after ovs has been started. The implementation adds two appctl commands: > > netdev-dpdk/attach and netdev-dpdk/detach. > > > > After the user attaches a new device, it can use the add-port command > > to use it in a switch, similarly, before detaching a device, it has to > > be removed using the del-port command. > > > > Signed-off-by: Mauricio Vasquez B < > mauricio.vasquezbernal@studenti.polito.it> > > --- > > I like the patch a lot, it really helps with the ease-of-use of > OVS. So, thanks very much for the work. > > > lib/netdev-dpdk.c | 79 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 79 insertions(+) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index f402354..c3a99e7 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void) > > } > > > > static void > > +netdev_dpdk_attach(struct unixctl_conn *conn, int argc, > > + const char *argv[], void *aux OVS_UNUSED) > ^ I think there's a spacing issue here. > > > +{ > > + int ret; > > + uint8_t port_id; > > + char response[512]; > > + > > + ovs_mutex_lock(&dpdk_mutex); > > + > > + ret = rte_eth_dev_attach(argv[1], &port_id); > > + if(ret < 0) { > ^ Spacing issue here > > > + snprintf(response, sizeof(response), > > + "Error attaching device ''", argv[1]); > ^ Spacing issue here > > > + unixctl_command_reply_error(conn, response); > > + goto unlock; > > + } > > + > > Does it make sense to also create the netdev at this point? > Personally, I think that it is better not to create it, I have these two reasons for the moment: 1. Creating a port does not only implies creating the netdev in netdev-dpdk, it implies creating the port in all the upper layers. It would be necessary to use ovsdb in order to do it, I don't know if it is allowed to use ovsdb in that way from a netdev-provider. 2. If the user invokes some external element (an orchestrator for example) that adds the port, after attaching the port, it could fail due to the port is already there. > > + snprintf(response, sizeof(response), > > + "Device '%s' has been attached as 'dpdk%d'", argv[1], > port_id); > > ^ Spacing issue here. > > > + unixctl_command_reply(conn, response); > > + > > +unlock: > > + ovs_mutex_unlock(&dpdk_mutex); > > +} > > + > > +static void > > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc, > > + const char *argv[], void *aux OVS_UNUSED) > ^ Spacing issue here. > > > +{ > > + int ret; > > + unsigned int port_id; > > + char name[RTE_ETH_NAME_MAX_LEN]; > > + char response[512]; > > + > > + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id); > > + if(ret) { > ^ Spacing issue here. > > > + snprintf(response, sizeof(response), > > + "'%s' is not a valid dpdk device", argv[1]); > ^ Spacing issue here. > > > + unixctl_command_reply_error(conn, response); > > + return; > > + } > > + > > + ovs_mutex_lock(&dpdk_mutex); > > + > > + struct netdev * netdev = netdev_from_name(argv[1]); > > + if(netdev) { > ^ Spacing issue here. > > > + netdev_close(netdev); > > + snprintf(response, sizeof(response), > > + "Port '%s' is being used. Remove it before > detaching", argv[1]); > > + unixctl_command_reply_error(conn, response); > > + goto unlock; > > + } > > + > > + rte_eth_dev_close(port_id); > > + > > + ret = rte_eth_dev_detach(port_id, name); > > + if(ret < 0) { > ^ Spacing issue here. > > > + snprintf(response, sizeof(response), > > + "Port '%s' can not be detached", argv[1]); > > + unixctl_command_reply_error(conn, response); > > + goto unlock; > > + } > > + > > + snprintf(response, sizeof(response), "Port '%s' has been detached", > argv[1]); > ^ Please stick to 80-bytes on a line. > > > + unixctl_command_reply(conn, response); > > + > > +unlock: > > + ovs_mutex_unlock(&dpdk_mutex); > > +} > > + > > +static void > > dpdk_common_init(void) > > { > > unixctl_command_register("netdev-dpdk/set-admin-state", > > "[netdev] up|down", 1, 2, > > netdev_dpdk_set_admin_state, NULL); > > > > + unixctl_command_register("netdev-dpdk/attach", > > + "device", 1, 1, > > + netdev_dpdk_attach, NULL); > > + > > + unixctl_command_register("netdev-dpdk/detach", > > + "port", 1, 1, > > + netdev_dpdk_detach, NULL); > > + > Does it make sense to have a single command ``netdev-dpdk/port-ctl'' > that takes either attach/detach as an argument? I think it looks a bit > better, and you might be able to combine a lot of the code above into a > single function, but I have been wrong on user-experience stuff before. > Yes, it makes a lot of sense, my only concern is that both parameters (attach and detach) would not receive the same type of argument, attach receives the pci address while detach receives the port name, however I think it is not a big problem, there are more advantages in creating a single command. I will send a proper patch updating also the documentation. > > > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); > > } > > Thanks again, > -Aaron > Mauricio V,
I sent a proper patch: http://openvswitch.org/pipermail/dev/2016-March/068600.html On Thu, Mar 24, 2016 at 11:57 AM, Mauricio Vásquez < mauricio.vasquezbernal@studenti.polito.it> wrote: > Hi Aaron, > > First of all thank you very much for your feedback. > > I didn't pay that attention to the style, I'll correct all the issues when > I send a proper patch, by the way, is there any script or tool to check the > style?, I looked for it and I cannot find anything. > > On Wed, Mar 23, 2016 at 11:42 AM, Aaron Conole <aconole@redhat.com> wrote: > >> Hi Mauricio, >> >> Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> writes: >> >> > In order to use dpdk ports in ovs they have to be bound to a DPDK >> > compatible driver before ovs is started. >> > >> > This patch adds the possibility to hotplug (or hot-unplug) a device >> > after ovs has been started. The implementation adds two appctl commands: >> > netdev-dpdk/attach and netdev-dpdk/detach. >> > >> > After the user attaches a new device, it can use the add-port command >> > to use it in a switch, similarly, before detaching a device, it has to >> > be removed using the del-port command. >> > >> > Signed-off-by: Mauricio Vasquez B < >> mauricio.vasquezbernal@studenti.polito.it> >> > --- >> >> I like the patch a lot, it really helps with the ease-of-use of >> OVS. So, thanks very much for the work. >> >> > lib/netdev-dpdk.c | 79 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 79 insertions(+) >> > >> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> > index f402354..c3a99e7 100644 >> > --- a/lib/netdev-dpdk.c >> > +++ b/lib/netdev-dpdk.c >> > @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void) >> > } >> > >> > static void >> > +netdev_dpdk_attach(struct unixctl_conn *conn, int argc, >> > + const char *argv[], void *aux OVS_UNUSED) >> ^ I think there's a spacing issue here. >> >> > +{ >> > + int ret; >> > + uint8_t port_id; >> > + char response[512]; >> > + >> > + ovs_mutex_lock(&dpdk_mutex); >> > + >> > + ret = rte_eth_dev_attach(argv[1], &port_id); >> > + if(ret < 0) { >> ^ Spacing issue here >> >> > + snprintf(response, sizeof(response), >> > + "Error attaching device ''", argv[1]); >> ^ Spacing issue here >> >> > + unixctl_command_reply_error(conn, response); >> > + goto unlock; >> > + } >> > + >> >> Does it make sense to also create the netdev at this point? >> > > Personally, I think that it is better not to create it, I have these two > reasons for the moment: > 1. Creating a port does not only implies creating the netdev in > netdev-dpdk, it implies creating the port in all the upper layers. It would > be necessary to use ovsdb in order to do it, I don't know if it is allowed > to use ovsdb in that way from a netdev-provider. > 2. If the user invokes some external element (an orchestrator for example) > that adds the port, after attaching the port, it could fail due to the port > is already there. > > >> > + snprintf(response, sizeof(response), >> > + "Device '%s' has been attached as 'dpdk%d'", argv[1], >> port_id); >> >> ^ Spacing issue here. >> >> > + unixctl_command_reply(conn, response); >> > + >> > +unlock: >> > + ovs_mutex_unlock(&dpdk_mutex); >> > +} >> > + >> > +static void >> > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc, >> > + const char *argv[], void *aux OVS_UNUSED) >> ^ Spacing issue here. >> >> > +{ >> > + int ret; >> > + unsigned int port_id; >> > + char name[RTE_ETH_NAME_MAX_LEN]; >> > + char response[512]; >> > + >> > + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id); >> > + if(ret) { >> ^ Spacing issue here. >> >> > + snprintf(response, sizeof(response), >> > + "'%s' is not a valid dpdk device", argv[1]); >> ^ Spacing issue here. >> >> > + unixctl_command_reply_error(conn, response); >> > + return; >> > + } >> > + >> > + ovs_mutex_lock(&dpdk_mutex); >> > + >> > + struct netdev * netdev = netdev_from_name(argv[1]); >> > + if(netdev) { >> ^ Spacing issue here. >> >> > + netdev_close(netdev); >> > + snprintf(response, sizeof(response), >> > + "Port '%s' is being used. Remove it before >> detaching", argv[1]); >> > + unixctl_command_reply_error(conn, response); >> > + goto unlock; >> > + } >> > + >> > + rte_eth_dev_close(port_id); >> > + >> > + ret = rte_eth_dev_detach(port_id, name); >> > + if(ret < 0) { >> ^ Spacing issue here. >> >> > + snprintf(response, sizeof(response), >> > + "Port '%s' can not be detached", argv[1]); >> > + unixctl_command_reply_error(conn, response); >> > + goto unlock; >> > + } >> > + >> > + snprintf(response, sizeof(response), "Port '%s' has been >> detached", argv[1]); >> ^ Please stick to 80-bytes on a line. >> >> > + unixctl_command_reply(conn, response); >> > + >> > +unlock: >> > + ovs_mutex_unlock(&dpdk_mutex); >> > +} >> > + >> > +static void >> > dpdk_common_init(void) >> > { >> > unixctl_command_register("netdev-dpdk/set-admin-state", >> > "[netdev] up|down", 1, 2, >> > netdev_dpdk_set_admin_state, NULL); >> > >> > + unixctl_command_register("netdev-dpdk/attach", >> > + "device", 1, 1, >> > + netdev_dpdk_attach, NULL); >> > + >> > + unixctl_command_register("netdev-dpdk/detach", >> > + "port", 1, 1, >> > + netdev_dpdk_detach, NULL); >> > + >> Does it make sense to have a single command ``netdev-dpdk/port-ctl'' >> that takes either attach/detach as an argument? I think it looks a bit >> better, and you might be able to combine a lot of the code above into a >> single function, but I have been wrong on user-experience stuff before. >> > > Yes, it makes a lot of sense, my only concern is that both parameters > (attach and detach) would not receive the same type of argument, attach > receives the pci address while detach receives the port name, however I > think it is not a big problem, there are more advantages in creating a > single command. > > I will send a proper patch updating also the documentation. > > >> >> > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); >> > } >> >> Thanks again, >> -Aaron >> > > Mauricio V, >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index f402354..c3a99e7 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void) } static void +netdev_dpdk_attach(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux OVS_UNUSED) +{ + int ret; + uint8_t port_id; + char response[512]; + + ovs_mutex_lock(&dpdk_mutex); + + ret = rte_eth_dev_attach(argv[1], &port_id); + if(ret < 0) { + snprintf(response, sizeof(response), + "Error attaching device ''", argv[1]); + unixctl_command_reply_error(conn, response); + goto unlock; + } + + snprintf(response, sizeof(response), + "Device '%s' has been attached as 'dpdk%d'", argv[1], port_id); + unixctl_command_reply(conn, response); + +unlock: + ovs_mutex_unlock(&dpdk_mutex); +} + +static void +netdev_dpdk_detach(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux OVS_UNUSED) +{ + int ret; + unsigned int port_id; + char name[RTE_ETH_NAME_MAX_LEN]; + char response[512]; + + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id); + if(ret) { + snprintf(response, sizeof(response), + "'%s' is not a valid dpdk device", argv[1]); + unixctl_command_reply_error(conn, response); + return; + } + + ovs_mutex_lock(&dpdk_mutex); + + struct netdev * netdev = netdev_from_name(argv[1]); + if(netdev) { + netdev_close(netdev); + snprintf(response, sizeof(response), + "Port '%s' is being used. Remove it before detaching", argv[1]); + unixctl_command_reply_error(conn, response); + goto unlock; + } + + rte_eth_dev_close(port_id); + + ret = rte_eth_dev_detach(port_id, name); + if(ret < 0) { + snprintf(response, sizeof(response), + "Port '%s' can not be detached", argv[1]); + unixctl_command_reply_error(conn, response); + goto unlock; + } + + snprintf(response, sizeof(response), "Port '%s' has been detached", argv[1]); + unixctl_command_reply(conn, response); + +unlock: + ovs_mutex_unlock(&dpdk_mutex); +} + +static void dpdk_common_init(void) { unixctl_command_register("netdev-dpdk/set-admin-state", "[netdev] up|down", 1, 2, netdev_dpdk_set_admin_state, NULL); + unixctl_command_register("netdev-dpdk/attach", + "device", 1, 1, + netdev_dpdk_attach, NULL); + + unixctl_command_register("netdev-dpdk/detach", + "port", 1, 1, + netdev_dpdk_detach, NULL); + ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); }
In order to use dpdk ports in ovs they have to be bound to a DPDK compatible driver before ovs is started. This patch adds the possibility to hotplug (or hot-unplug) a device after ovs has been started. The implementation adds two appctl commands: netdev-dpdk/attach and netdev-dpdk/detach. After the user attaches a new device, it can use the add-port command to use it in a switch, similarly, before detaching a device, it has to be removed using the del-port command. Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> --- lib/netdev-dpdk.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)