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.
> > + 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?
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
