[ovs-dev] [PATCH v4] netdev-dpdk: add hotplug support
Mauricio Vásquez
mauricio.vasquezbernal at studenti.polito.it
Wed May 11 16:53:12 UTC 2016
Hi Flavio,
On Tue, May 10, 2016 at 10:11 PM, Flavio Leitner <fbl at 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.vasquezbernal at 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.vasquezbernal at 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 bugs at 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 at openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
>
More information about the dev
mailing list