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