[ovs-dev] [PATCH 1/3] netdev-dpdk: add hotplug support

[Mauricio Vásquez]

Hi Ian,

On Thu, Mar 31, 2016 at 2:02 PM, Stokes, Ian <ian.stokes at intel.com> wrote:

> Hi Mauricio,
>
> This patch is quite useful. Some minor comments inline. I've also tested
> the patch and can confirm it works without issue.
>

Great!


>
> Thanks
> Ian
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Mauricio
> > Vasquez B
> > Sent: Monday, March 28, 2016 9:52 AM
> > To: dev at openvswitch.org
> > Subject: [ovs-dev] [PATCH 1/3] netdev-dpdk: add hotplug support
> >
> > 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 to use 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>
> > ---
> >  lib/netdev-dpdk.c | 73
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 7c4cd07..05fa0df 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1982,6 +1982,75 @@ netdev_dpdk_set_admin_state(struct unixctl_conn
> > *conn, int argc,
> >      unixctl_command_reply(conn, "OK");
> >  }
> >
> > +static void
> > +netdev_dpdk_port_ctl(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > +                     const char *argv[], void *aux OVS_UNUSED) {
> > +    int ret;
> > +    uint8_t port_id;
> > +    unsigned int parsed_port;
> > +    char devname[RTE_ETH_NAME_MAX_LEN];
> > +    char response[512];
> > +
> > +    ovs_mutex_lock(&dpdk_mutex);
> > +
> > +    if (strcmp(argv[1], "attach") == 0) {
> > +        ret = rte_eth_dev_attach(argv[2], &port_id);
> > +        if (ret < 0) {
> > +            snprintf(response, sizeof(response),
> > +                     "Error attaching device '%s'", argv[2]);
> > +            unixctl_command_reply_error(conn, response);
> > +            goto unlock;
> > +        }
> > +
> > +        snprintf(response, sizeof(response),
> > +                 "Device '%s' has been attached as 'dpdk%d'", argv[2],
> > port_id);
> > +        unixctl_command_reply(conn, response);
> > +
> > +    } else if (strcmp(argv[1], "detach") == 0) {
> > +        ret = dpdk_dev_parse_name(argv[2], "dpdk", &parsed_port);
> > +        if (ret) {
> > +            snprintf(response, sizeof(response),
> > +                     "'%s' is not a valid dpdk device", argv[2]);
> > +            unixctl_command_reply_error(conn, response);
> > +            goto unlock;
> > +        }
> > +
> > +        port_id = parsed_port;
> > +
>
> Very minor style change here, the extra space between '*' and 'netdev'
> below can be removed.
>

I will correct it in a future v2.


>
> > +        struct netdev * netdev = netdev_from_name(argv[2]);
> > +        if (netdev) {
>
> So we should only enter here if the netdev device exists?
>

Yes, it is because if the device is being used it can not be detached
because it will cause a crash on the PMD threads that are using the device.


> However is there a specific reason you call netdev_close() before
> reporting the device is busy?
> I've tested with and without the call below and didn’t notice any
> functional difference; the port was still able to send/receive traffic.
> In the case the device is busy, is it required? If busy should the device
> be left as is and the reply error logged?
>

netdev_from_name() increases the count reference for that device, then
netdev_close() is called just to decrease the ref_cnt to the original value
avoiding a memory leakage.


>
> > +            netdev_close(netdev);
> > +            snprintf(response, sizeof(response),
> > +                     "Port '%s' is being used. Remove it before
> > detaching",
> > +                     argv[2]);
> > +            unixctl_command_reply_error(conn, response);
> > +            goto unlock;
> > +        }
> > +
> > +        rte_eth_dev_close(port_id);
> > +
> > +        ret = rte_eth_dev_detach(port_id, devname);
> > +        if (ret < 0) {
> > +            snprintf(response, sizeof(response),
> > +                     "Port '%s' can not be detached", argv[2]);
> > +            unixctl_command_reply_error(conn, response);
> > +            goto unlock;
> > +        }
> > +
> > +        snprintf(response, sizeof(response),
> > +                 "Port '%s' has been detached", argv[2]);
> > +        unixctl_command_reply(conn, response);
> > +    } else {
> > +        snprintf(response, sizeof(response),
> > +                 "'%s' is not a valid argument", argv[1]);
> > +        unixctl_command_reply_error(conn, response);
> > +    }
> > +
> > +unlock:
> > +    ovs_mutex_unlock(&dpdk_mutex);
> > +}
> > +
> >  /*
> >   * Set virtqueue flags so that we do not receive interrupts.
> >   */
> > @@ -2262,6 +2331,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);
> > +
> >      ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);  }
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>

Thanks for the feedback,

Mauricio Vasquez,