Hello, I sent v5: http://openvswitch.org/pipermail/dev/2016-May/070953.html
Mauricio V, On Wed, May 11, 2016 at 6:53 PM, Mauricio Vásquez < mauricio.vasquezber...@studenti.polito.it> wrote: > Hi Flavio, > > On Tue, May 10, 2016 at 10:11 PM, Flavio Leitner <f...@sysclose.org> wrote: > >> >> Thanks for this patch, Mauricio. >> More inline. >> >> On Tue, May 10, 2016 at 09:47:12PM +0200, Mauricio Vásquez wrote: >> > Adding in CC all the people involved. >> > (I don't know why git just takes two) >> > >> > On Tue, May 10, 2016 at 9:11 PM, Mauricio Vasquez B < >> > mauricio.vasquezber...@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 an appctl command: >> > > netdev-dpdk/port-clt >> > > >> > > After the user attaches a new device, it has to be added to a bridge >> > > using the add-port command, similarly, before detaching a device >> > > it has to be removed using the del-port command. >> > > >> > > Signed-off-by: Mauricio Vasquez B < >> > > mauricio.vasquezber...@studenti.polito.it> >> > > --- >> > > v4: >> > > - fix typo in commit message >> > > - remove unnecessary whitespace change in INSTALL.DPDK.md >> > > v3: >> > > - create dpdk_port_attach and dpdk_port_detach functions >> > > - modify mutex locking order >> > > v2: >> > > - use rte_eth_dev_is_valid_port() to check if a port is valid >> > > >> > > INSTALL.DPDK.md | 24 +++++++++++++ >> > > NEWS | 1 + >> > > lib/netdev-dpdk.c | 102 >> > > ++++++++++++++++++++++++++++++++++++++++++++++++++---- >> > > 3 files changed, 120 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md >> > > index 9ec8bf6..026373d 100644 >> > > --- a/INSTALL.DPDK.md >> > > +++ b/INSTALL.DPDK.md >> > > @@ -227,6 +227,29 @@ Using the DPDK with ovs-vswitchd: >> > > For more details regarding egress-policer parameters please refer >> to >> > > the >> > > vswitch.xml. >> > > >> > > +9. Port Hotplug >> > > + >> > > + ovs supports port hotplugging, it allows to use ports that were >> not >> > > bound >> > > + to DPDK when vswitchd was started. >> > > + In order to attach a port, it has to be bound to DPDK using the >> > > + dpdk_nic_bind.py script: >> > > + >> > > + `$DPDK_DIR/tools/dpdk_nic_bind.py --bind=igb_uio 0000:01:00.0` >> > > + >> > > + Then it can be attached to OVS: >> > > + >> > > + `ovs-appctl netdev-dpdk/port-ctl attach 0000:01:00.0` >> > > + >> > > + At this point, the user can create a ovs port using the add-port >> > > command. >> > > + >> > > + It is also possible to detach a port from ovs, the user has to >> remove >> > > the >> > > + port using the del-port command, then it can be detached using: >> > > + >> > > + `ovs-appctl netdev-dpdk/port-ctl detach dpdk0` >> > > + >> > > + This feature is not supported by all the NICs, please refer to the >> > > + [DPDK Port Hotplug Framework] in order to get more information. >> > > + >> > > Performance Tuning: >> > > ------------------- >> > > >> > > @@ -959,3 +982,4 @@ Please report problems to b...@openvswitch.org. >> > > [INSTALL.md]:INSTALL.md >> > > [DPDK Linux GSG]: >> > > >> http://www.dpdk.org/doc/guides/linux_gsg/build_dpdk.html#binding-and-unbinding-network-ports-to-from-the-igb-uioor-vfio-modules >> > > [DPDK Docs]: http://dpdk.org/doc >> > > +[DPDK Port Hotplug Framework]: >> > > http://dpdk.org/doc/guides/prog_guide/port_hotplug_framework.html >> > > diff --git a/NEWS b/NEWS >> > > index ea7f3a1..2ba8659 100644 >> > > --- a/NEWS >> > > +++ b/NEWS >> > > @@ -26,6 +26,7 @@ Post-v2.5.0 >> > > assignment. >> > > * Type of log messages from PMD threads changed from INFO to >> DBG. >> > > * QoS functionality with sample egress-policer implementation. >> > > + * Port Hotplug is now supported. >> > > - ovs-benchmark: This utility has been removed due to lack of use >> and >> > > bitrot. >> > > - ovs-appctl: >> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> > > index e09b471..cd98a2b 100644 >> > > --- a/lib/netdev-dpdk.c >> > > +++ b/lib/netdev-dpdk.c >> > > @@ -599,7 +599,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev) >> > > OVS_REQUIRES(dpdk_mutex) >> > > int diag; >> > > int n_rxq, n_txq; >> > > >> > > - if (dev->port_id < 0 || dev->port_id >= rte_eth_dev_count()) { >> > > + if (!rte_eth_dev_is_valid_port(dev->port_id)) { >> > > return ENODEV; >> > > } >> > > >> > > @@ -1985,6 +1985,89 @@ netdev_dpdk_set_admin_state(struct unixctl_conn >> > > *conn, int argc, >> > > unixctl_command_reply(conn, "OK"); >> > > } >> > > >> > > +static int >> > > +dpdk_port_attach(const char * port, char * response, size_t size) >> >> minor nit: extra space after '*'. Look at other functions in the >> same file. >> >> >> > > + OVS_REQUIRES(dpdk_mutex) >> > > +{ >> > > + int ret; >> > > + uint8_t port_id; >> > > + >> > > + ret = rte_eth_dev_attach(port, &port_id); >> > > + if (ret < 0) { >> > > + snprintf(response, size, "Error attaching device '%s'", >> port); >> > > + return -1; >> > > + } >> > > + >> > > + snprintf(response, size, >> > > + "Device '%s' has been attached as 'dpdk%d'", port, >> port_id); >> > > + return 0; >> > > +} >> > > + >> > > +static int >> > > +dpdk_port_detach(const char * port, char * response, size_t size) >> >> same here. >> > > I'll correct them in the next version. > > >> >> > > + OVS_REQUIRES(dpdk_mutex) >> > > +{ >> > > + int ret; >> > > + unsigned int parsed_port; >> > > + uint8_t port_id; >> > > + char devname[RTE_ETH_NAME_MAX_LEN]; >> > > + >> > > + ret = dpdk_dev_parse_name(port, "dpdk", &parsed_port); >> > > + if (ret) { >> > > + snprintf(response, size, "'%s' is not a valid dpdk device", >> port); >> > > + return -1; >> > > + } >> > > + >> > > + port_id = parsed_port; >> > > + >> > > + struct netdev *netdev = netdev_from_name(port); >> > > + if (netdev) { >> > > + netdev_close(netdev); >> > > + snprintf(response, size, >> > > + "Port '%s' is being used. Remove it before >> detaching", >> > > port); >> > > + return -1; >> > > + } >> > > + >> > > + rte_eth_dev_close(port_id); >> > > + >> > > + ret = rte_eth_dev_detach(port_id, devname); >> > > + if (ret < 0) { >> > > + snprintf(response, size, "Port '%s' can not be detached", >> port); >> > > + return -1; >> > > + } >> > > + >> > > + snprintf(response, size, "Port '%s' has been detached", port); >> > > + return 0; >> > > +} >> > > + >> > > +static void >> > > +netdev_dpdk_port_ctl(struct unixctl_conn *conn, int argc OVS_UNUSED, >> > > + const char *argv[], void *aux OVS_UNUSED) >> > > +{ >> > > + int ret; >> > > + char response[512]; >> > > + >> > > + ovs_mutex_lock(&dpdk_mutex); >> > > + >> > > + if (strcmp(argv[1], "attach") == 0) { >> > > + ret = dpdk_port_attach(argv[2], response, sizeof(response)); >> > > + } else if (strcmp(argv[1], "detach") == 0) { >> > > + ret = dpdk_port_detach(argv[2], response, sizeof(response)); >> > > + } else { >> > > + snprintf(response, sizeof(response), >> > > + "'%s' is not a valid argument", argv[1]); >> > > + ret = -1; >> > > + } >> > > + >> > > + ovs_mutex_unlock(&dpdk_mutex); >> > > + >> > > + if (ret) { >> > > + unixctl_command_reply_error(conn, response); >> > > + } else { >> > > + unixctl_command_reply(conn, response); >> > > + } >> > > +} >> > > + >> > > /* >> > > * Set virtqueue flags so that we do not receive interrupts. >> > > */ >> > > @@ -2282,6 +2365,10 @@ dpdk_common_init(void) >> > > "[netdev] up|down", 1, 2, >> > > netdev_dpdk_set_admin_state, NULL); >> > > >> > > + unixctl_command_register("netdev-dpdk/port-ctl", >> > > + "attach/detach device", 2, 2, >> > > + netdev_dpdk_port_ctl, NULL); >> > > + >> >> Not sure about the command name because we already have few patterns >> established and that would be another different one. >> >> For instance tnl and route uses cmd namespaces like: >> <module>/<subset>/<action> [parameters] >> I.e.: >> ovs/route/add ip_addr/prefix_len out_br_name gw >> ovs/route/del ip_addr/prefix_len >> ovs/route/lookup ip_addr >> >> >> There is also the <module><action-subset> or <subset-action>. >> I.e.: >> bond/set-active-slave port slave >> bond/disable-slave port slave >> bond/enable-slave port slave >> >> dpif-netdev/pmd-rxq-show [dp] >> dpif-netdev/pmd-stats-clear [dp] >> dpif-netdev/pmd-stats-show [dp] >> >> I'd prefer to stick with one of the existent patterns: >> E.g.: >> >> netdev-dpdk/port/attach >> netdev-dpdk/port/detach >> but in this case I don't know about set-admin-state which would >> have be in netdev-dpdk/port/set-admin-state instead. >> >> or maybe simply: >> netdev-dpdk/attach-port >> netdev-dpdk/detach-port >> >> What do you think? >> > > After viewing the examples your provided I think the best approach is > netdev-dpdk/attach-port > netdev-dpdk/detach-port > > I will include that in the new version. > > Thanks for reviewing! > > >> >> Otherwise the patch looks good to me. >> I plan to test it tomorrow. >> >> fbl >> >> > > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); >> > > } >> > > >> > > @@ -2293,7 +2380,7 @@ dpdk_ring_create(const char dev_name[], >> unsigned int >> > > port_no, >> > > { >> > > struct dpdk_ring *ivshmem; >> > > char ring_name[RTE_RING_NAMESIZE]; >> > > - int err; >> > > + int err, port_id; >> > > >> > > ivshmem = dpdk_rte_mzalloc(sizeof *ivshmem); >> > > if (ivshmem == NULL) { >> > > @@ -2327,19 +2414,20 @@ dpdk_ring_create(const char dev_name[], >> unsigned >> > > int port_no, >> > > return ENOMEM; >> > > } >> > > >> > > - err = rte_eth_from_rings(dev_name, &ivshmem->cring_rx, 1, >> > > - &ivshmem->cring_tx, 1, SOCKET0); >> > > + port_id = rte_eth_from_rings(dev_name, &ivshmem->cring_rx, 1, >> > > + &ivshmem->cring_tx, 1, SOCKET0); >> > > >> > > - if (err < 0) { >> > > + if (port_id < 0) { >> > > rte_free(ivshmem); >> > > return ENODEV; >> > > } >> > > >> > > ivshmem->user_port_id = port_no; >> > > - ivshmem->eth_port_id = rte_eth_dev_count() - 1; >> > > + ivshmem->eth_port_id = port_id; >> > > + *eth_port_id = port_id; >> > > + >> > > ovs_list_push_back(&dpdk_ring_list, &ivshmem->list_node); >> > > >> > > - *eth_port_id = ivshmem->eth_port_id; >> > > return 0; >> > > } >> > > >> > > -- >> > > 1.9.1 >> > > >> > > _______________________________________________ >> > > dev mailing list >> > > dev@openvswitch.org >> > > http://openvswitch.org/mailman/listinfo/dev >> > > >> > _______________________________________________ >> > dev mailing list >> > dev@openvswitch.org >> > http://openvswitch.org/mailman/listinfo/dev >> >> >> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
