[ovs-dev] [PATCH v5 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming

Daniele Di Proietto diproiettod at ovn.org
Fri Jan 6 04:15:05 UTC 2017


If new_port_id == -1 I think it's better to return failure from
set_config(), rather than return failure from a later reconfigure(),
so I squashed the following incremental:

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e66ff2711..038f79b80 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1231,12 +1231,13 @@ netdev_dpdk_set_config(struct netdev *netdev,
const struct smap *args)
          * is valid */
         if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
                && rte_eth_dev_is_valid_port(dev->port_id))) {
-            err = EINVAL;
             int new_port_id = netdev_dpdk_process_devargs(new_devargs);
-            if (new_port_id == dev->port_id) {
+            if (!rte_eth_dev_is_valid_port(new_port_id)) {
+                err = EINVAL;
+            } else if (new_port_id == dev->port_id) {
                 /* Already configured, do not reconfigure again */
                 err = 0;
-            } else if (rte_eth_dev_is_valid_port(new_port_id)) {
+            } else {
                 struct netdev_dpdk *dup_dev;
                 dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
                 if (dup_dev) {

and applied to master, thanks!

2017-01-05 2:42 GMT-08:00 Ciara Loftus <ciara.loftus at intel.com>:
> 'dpdk' ports no longer have naming restrictions. Now, instead of
> specifying the dpdk port ID as part of the name, the PCI address of the
> device must be specified via the 'dpdk-devargs' option. eg.
>
> ovs-vsctl add-port br0 my-port
> ovs-vsctl set Interface my-port type=dpdk
>   options:dpdk-devargs=0000:06:00.3
>
> The user must no longer hotplug attach DPDK ports by issuing the
> specific ovs-appctl netdev-dpdk/attach command. The hotplug is now
> automatically invoked when a valid PCI address is set in the
> dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command
> has changed in that the user now must specify the relevant PCI address
> as input instead of the port name.
>
> Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
> Co-authored-by: Daniele Di Proietto <diproiettod at vmware.com>
> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> Co-authored-by: Kevin Traynor <ktraynor at redhat.com>
> ---
> Changelog:
> * Keep port-detach appctl function - use PCI as input arg
> * Add requires_mutex to devargs processing functions
> * use reconfigure infrastructure for devargs changes
> * process devargs even if valid portid ie. device already configured
> * report err if dpdk-devargs is not specified
> * Add Daniele's incremental patch & Sign-off + Co-author tags
> * Update details of detach command to reflect PCI is needed instead of
>   port name
> * Update NEWS to mention that the new naming scheme is not backwards
>   compatible
> * Use existing DPDk functions to get port ID from PCI address/devname
> * Merged process_devargs and process_pdevargs functions
> * Removed unnecessary requires_mutex from devargs processing fn
> * Fix case where port is re-attached after detach
> * Add note to documentation that devices won't work until devargs set.
> * Set netdev type and dpdk-devargs in one command in docs to avoid
>   errors.
> * Change port names in documentation to emphasise arbitrary-ness.
> * Add Kevin's incremental & sign-off/co-authored-by
> * Check if devargs string has changed before processing it as suggested
>   by Daniele.
> * Print error if attach fails
>
>  Documentation/howto/dpdk.rst         |   7 +-
>  Documentation/intro/install/dpdk.rst |   9 +-
>  NEWS                                 |   5 +
>  lib/netdev-dpdk.c                    | 181 ++++++++++++++++++++++++-----------
>  vswitchd/vswitch.xml                 |   8 ++
>  5 files changed, 148 insertions(+), 62 deletions(-)
>
> diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
> index 13da548..1ff672c 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -312,14 +312,13 @@ In order to attach a port, it has to be bound to DPDK using the
>
>  Then it can be attached to OVS::
>
> -    $ ovs-appctl netdev-dpdk/attach 0000:01:00.0
> -
> -At this point, the user can create a dpdk port using the ``add-port`` command.
> +    $ ovs-vsctl add-port br0 dpdkx -- set Interface dpdkx type=dpdk \
> +        options:dpdk-devargs=0000:01:00.0
>
>  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/detach dpdk0
> +    $ ovs-appctl netdev-dpdk/detach dpdkx
>
>  This feature is not supported with VFIO and does not work with some NICs.
>  For more information please refer to the `DPDK Port Hotplug Framework
> diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
> index 10ceabb..fff0a1a 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -252,8 +252,13 @@ ports. For example, to create a userspace bridge named ``br0`` and add two
>  ``dpdk`` ports to it, run::
>
>      $ ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev
> -    $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> -    $ ovs-vsctl add-port br0 dpdk1 -- set Interface dpdk1 type=dpdk
> +    $ ovs-vsctl add-port br0 myportnameone -- set Interface myportnameone \
> +        type=dpdk options:dpdk-devargs=0000:06:00.0
> +    $ ovs-vsctl add-port br0 myportnametwo -- set Interface myportnametwo \
> +        type=dpdk options:dpdk-devargs=0000:06:00.1
> +
> +DPDK devices will not be available for use until a valid dpdk-devargs is
> +specified.
>
>  Refer to ovs-vsctl(8) and :doc:`/howto/dpdk` for more details.
>
> diff --git a/NEWS b/NEWS
> index 8f8c2f7..d66d402 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -48,6 +48,11 @@ Post-v2.6.0
>       * Support for DPDK v16.11.
>       * Support for rx checksum offload. Refer DPDK HOWTO for details.
>       * Port Hotplug is now supported.
> +     * DPDK physical ports can now have arbitrary names. The PCI address of
> +       the device must be set using the 'dpdk-devargs' option. Compatibility
> +       with the old dpdk<portid> naming scheme is broken, and as such a
> +       device will not be available for use until a valid dpdk-devargs is
> +       specified.
>     - Fedora packaging:
>       * A package upgrade does not automatically restart OVS service.
>     - ovs-vswitchd/ovs-vsctl:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index b716b57..e66ff27 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -356,6 +356,9 @@ struct netdev_dpdk {
>      /* Identifier used to distinguish vhost devices from each other. */
>      char vhost_id[PATH_MAX];
>
> +    /* Device arguments for dpdk ports */
> +    char *devargs;
> +
>      /* In dpdk_list. */
>      struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>
> @@ -846,7 +849,7 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int port_no,
>      /* If the 'sid' is negative, it means that the kernel fails
>       * to obtain the pci numa info.  In that situation, always
>       * use 'SOCKET0'. */
> -    if (type == DPDK_DEV_ETH) {
> +    if (type == DPDK_DEV_ETH && rte_eth_dev_is_valid_port(dev->port_id)) {
>          sid = rte_eth_dev_socket_id(port_no);
>      } else {
>          sid = rte_lcore_to_socket_id(rte_get_master_lcore());
> @@ -888,9 +891,11 @@ netdev_dpdk_init(struct netdev *netdev, unsigned int port_no,
>      /* Initilize the hardware offload flags to 0 */
>      dev->hw_ol_features = 0;
>      if (type == DPDK_DEV_ETH) {
> -        err = dpdk_eth_dev_init(dev);
> -        if (err) {
> -            goto unlock;
> +        if (rte_eth_dev_is_valid_port(dev->port_id)) {
> +            err = dpdk_eth_dev_init(dev);
> +            if (err) {
> +                goto unlock;
> +            }
>          }
>          dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
>      } else {
> @@ -986,17 +991,10 @@ netdev_dpdk_vhost_client_construct(struct netdev *netdev)
>  static int
>  netdev_dpdk_construct(struct netdev *netdev)
>  {
> -    unsigned int port_no;
>      int err;
>
> -    /* Names always start with "dpdk" */
> -    err = dpdk_dev_parse_name(netdev->name, "dpdk", &port_no);
> -    if (err) {
> -        return err;
> -    }
> -
>      ovs_mutex_lock(&dpdk_mutex);
> -    err = netdev_dpdk_init(netdev, port_no, DPDK_DEV_ETH);
> +    err = netdev_dpdk_init(netdev, -1, DPDK_DEV_ETH);
>      ovs_mutex_unlock(&dpdk_mutex);
>      return err;
>  }
> @@ -1010,6 +1008,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
>      ovs_mutex_lock(&dev->mutex);
>
>      rte_eth_dev_stop(dev->port_id);
> +    free(dev->devargs);
>      free(ovsrcu_get_protected(struct ingress_policer *,
>                                &dev->ingress_policer));
>
> @@ -1117,6 +1116,49 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>      return 0;
>  }
>
> +static struct netdev_dpdk *
> +netdev_dpdk_lookup_by_port_id(int port_id)
> +    OVS_REQUIRES(dpdk_mutex)
> +{
> +    struct netdev_dpdk *dev;
> +
> +    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> +        if (dev->port_id == port_id) {
> +            return dev;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static int
> +netdev_dpdk_process_devargs(const char *devargs)
> +{
> +    struct rte_pci_addr addr;
> +    uint8_t new_port_id = UINT8_MAX;
> +
> +    if (!eal_parse_pci_DomBDF(devargs, &addr)) {
> +        /* Valid PCI address format detected - configure physical device */
> +        if (!rte_eth_dev_count()
> +                || rte_eth_dev_get_port_by_name(devargs, &new_port_id)
> +                || !rte_eth_dev_is_valid_port(new_port_id)) {
> +            /* PCI device not found in DPDK, attempt to attach it */
> +            if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> +                /* Attach successful */
> +                VLOG_INFO("Device "PCI_PRI_FMT" has been attached to DPDK",
> +                          addr.domain, addr.bus, addr.devid, addr.function);
> +            } else {
> +                /* Attach unsuccessful */
> +                VLOG_INFO("Error attaching device "PCI_PRI_FMT" to DPDK",
> +                          addr.domain, addr.bus, addr.devid, addr.function);
> +                return -1;
> +            }
> +        }
> +    }
> +
> +    return new_port_id;
> +}
> +
>  static void
>  dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args)
>      OVS_REQUIRES(dev->mutex)
> @@ -1159,7 +1201,10 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
>      };
>      bool rx_chksm_ofld;
>      bool temp_flag;
> +    const char *new_devargs;
> +    int err = 0;
>
> +    ovs_mutex_lock(&dpdk_mutex);
>      ovs_mutex_lock(&dev->mutex);
>
>      dpdk_set_rxq_config(dev, args);
> @@ -1171,6 +1216,52 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
>                              NIC_PORT_DEFAULT_TXQ_SIZE,
>                              &dev->requested_txq_size);
>
> +    new_devargs = smap_get(args, "dpdk-devargs");
> +
> +    if (dev->devargs && strcmp(new_devargs, dev->devargs)) {
> +        /* The user requested a new device.  If we return error, the caller
> +         * will delete this netdev and try to recreate it. */
> +        err = EAGAIN;
> +        goto out;
> +    }
> +
> +    /* dpdk-devargs is required for device configuration */
> +    if (new_devargs && new_devargs[0]) {
> +        /* Don't process dpdk-devargs if value is unchanged and port id
> +         * is valid */
> +        if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
> +               && rte_eth_dev_is_valid_port(dev->port_id))) {
> +            err = EINVAL;
> +            int new_port_id = netdev_dpdk_process_devargs(new_devargs);
> +            if (new_port_id == dev->port_id) {
> +                /* Already configured, do not reconfigure again */
> +                err = 0;
> +            } else if (rte_eth_dev_is_valid_port(new_port_id)) {
> +                struct netdev_dpdk *dup_dev;
> +                dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
> +                if (dup_dev) {
> +                    VLOG_WARN("'%s' is trying to use device '%s' which is "
> +                              "already in use by '%s'.",
> +                              netdev_get_name(netdev), new_devargs,
> +                              netdev_get_name(&dup_dev->up));
> +                    err = EADDRINUSE;
> +                } else {
> +                    dev->devargs = xstrdup(new_devargs);
> +                    dev->port_id = new_port_id;
> +                    netdev_request_reconfigure(&dev->up);
> +                    err = 0;
> +                }
> +            }
> +        }
> +    } else {
> +        /* dpdk-devargs unspecified - can't configure device */
> +        err = EINVAL;
> +    }
> +
> +    if (err) {
> +        goto out;
> +    }
> +
>      rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>      tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>      autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> @@ -1191,9 +1282,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args)
>          dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD;
>          dpdk_eth_checksum_offload_configure(dev);
>      }
> +
> +out:
>      ovs_mutex_unlock(&dev->mutex);
> +    ovs_mutex_unlock(&dpdk_mutex);
>
> -    return 0;
> +    return err;
>  }
>
>  static int
> @@ -2358,57 +2452,35 @@ netdev_dpdk_set_admin_state(struct unixctl_conn *conn, int argc,
>  }
>
>  static void
> -netdev_dpdk_attach(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                   const char *argv[], void *aux OVS_UNUSED)
> -{
> -    int ret;
> -    char *response;
> -    uint8_t port_id;
> -
> -    ovs_mutex_lock(&dpdk_mutex);
> -
> -    ret = rte_eth_dev_attach(argv[1], &port_id);
> -    if (ret < 0) {
> -        response = xasprintf("Error attaching device '%s'", argv[1]);
> -        ovs_mutex_unlock(&dpdk_mutex);
> -        unixctl_command_reply_error(conn, response);
> -        free(response);
> -        return;
> -    }
> -
> -    response = xasprintf("Device '%s' has been attached as 'dpdk%d'",
> -                         argv[1], port_id);
> -
> -    ovs_mutex_unlock(&dpdk_mutex);
> -    unixctl_command_reply(conn, response);
> -    free(response);
> -}
> -
> -static void
>  netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                     const char *argv[], void *aux OVS_UNUSED)
>  {
>      int ret;
>      char *response;
> -    unsigned int parsed_port;
>      uint8_t port_id;
>      char devname[RTE_ETH_NAME_MAX_LEN];
> +    struct rte_pci_addr addr;
> +    struct netdev_dpdk *dev;
>
>      ovs_mutex_lock(&dpdk_mutex);
>
> -    ret = dpdk_dev_parse_name(argv[1], "dpdk", &parsed_port);
> -    if (ret) {
> -        response = xasprintf("'%s' is not a valid port", argv[1]);
> +    if (eal_parse_pci_DomBDF(argv[1], &addr)) {
> +        response = xasprintf("Invalid PCI address '%s'. Cannot detach.",
> +                             argv[1]);
>          goto error;
>      }
>
> -    port_id = parsed_port;
> +    if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1],
> +                                                             &port_id)) {
> +        response = xasprintf("Device '%s' not found in DPDK", argv[1]);
> +        goto error;
> +    }
>
> -    struct netdev *netdev = netdev_from_name(argv[1]);
> -    if (netdev) {
> -        netdev_close(netdev);
> -        response = xasprintf("Port '%s' is being used. Remove it before"
> -                             "detaching", argv[1]);
> +    dev = netdev_dpdk_lookup_by_port_id(port_id);
> +    if (dev) {
> +        response = xasprintf("Device '%s' is being used by interface '%s'. "
> +                             "Remove it before detaching",
> +                             argv[1], netdev_get_name(&dev->up));
>          goto error;
>      }
>
> @@ -2416,11 +2488,11 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>      ret = rte_eth_dev_detach(port_id, devname);
>      if (ret < 0) {
> -        response = xasprintf("Port '%s' can not be detached", argv[1]);
> +        response = xasprintf("Device '%s' can not be detached", argv[1]);
>          goto error;
>      }
>
> -    response = xasprintf("Port '%s' has been detached", argv[1]);
> +    response = xasprintf("Device '%s' has been detached", argv[1]);
>
>      ovs_mutex_unlock(&dpdk_mutex);
>      unixctl_command_reply(conn, response);
> @@ -2708,11 +2780,8 @@ netdev_dpdk_class_init(void)
>          unixctl_command_register("netdev-dpdk/set-admin-state",
>                                   "[netdev] up|down", 1, 2,
>                                   netdev_dpdk_set_admin_state, NULL);
> -        unixctl_command_register("netdev-dpdk/attach",
> -                                 "pci address of device", 1, 1,
> -                                 netdev_dpdk_attach, NULL);
>          unixctl_command_register("netdev-dpdk/detach",
> -                                 "port", 1, 1,
> +                                 "pci address of device", 1, 1,
>                                   netdev_dpdk_detach, NULL);
>
>          ovsthread_once_done(&once);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index e73f184..c34c295 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2303,6 +2303,14 @@
>          </p>
>        </column>
>
> +      <column name="options" key="dpdk-devargs"
> +              type='{"type": "string"}'>
> +        <p>
> +          Specifies the PCI address of a physical dpdk device.
> +          Only supported by 'dpdk' devices.
> +        </p>
> +      </column>
> +
>        <column name="other_config" key="pmd-rxq-affinity">
>          <p>Specifies mapping of RX queues of this interface to CPU cores.</p>
>          <p>Value should be set in the following form:</p>
> --
> 2.4.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list