[ovs-dev,v3] netdev-dpdk: fix port addition for ports sharing same PCI id

Message ID 1515553529-5857-1-git-send-email-yliu@fridaylinux.org
State Accepted
Delegated to: Ian Stokes
Headers show
Series
  • [ovs-dev,v3] netdev-dpdk: fix port addition for ports sharing same PCI id
Related show

Commit Message

Yuanhan Liu Jan. 10, 2018, 3:05 a.m.
Some NICs have only one PCI address associated with multiple ports. This
patch extends the dpdk-devargs option's format to cater for such devices.

To achieve that, this patch uses a new syntax that will be adapted and
implemented in future DPDK release (likely, v18.05):
    http://dpdk.org/ml/archives/dev/2017-December/084234.html

And since it's the DPDK duty to parse the (complete and full) syntax
and this patch is more likely to serve as an intermediate workaround,
here I take a simpler and shorter syntax from it (note it's allowed to
have only one category being provided):
    class=eth,mac=00:11:22:33:44:55:66

Also, old compatibility is kept. Users can still go on with using the
PCI id to add a port (if that's enough for them). Meaning, this patch
will not break anything.

This patch is basically based on the one from Ciara:
    https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339496.html

Cc: Loftus Ciara <ciara.loftus@intel.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
---

v3: - reuse eth helper functions from OVS, suggested by Ilya Maximets

v2: - added doc for the new devags syntax
    - added error log for wrong mac format
---
 Documentation/howto/dpdk.rst | 12 ++++++++
 lib/netdev-dpdk.c            | 70 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 67 insertions(+), 15 deletions(-)

Comments

Ian Stokes Jan. 19, 2018, 11:03 a.m. | #1
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Wednesday, January 10, 2018 3:05 AM
> To: dev@openvswitch.org
> Cc: Stokes, Ian <ian.stokes@intel.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Yuanhan Liu <yliu@fridaylinux.org>; Loftus, Ciara
> <ciara.loftus@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Kevin
> Traynor <ktraynor@redhat.com>
> Subject: [PATCH v3] netdev-dpdk: fix port addition for ports sharing same
> PCI id
> 
> Some NICs have only one PCI address associated with multiple ports. This
> patch extends the dpdk-devargs option's format to cater for such devices.
> 
> To achieve that, this patch uses a new syntax that will be adapted and
> implemented in future DPDK release (likely, v18.05):
>     http://dpdk.org/ml/archives/dev/2017-December/084234.html
> 
> And since it's the DPDK duty to parse the (complete and full) syntax and
> this patch is more likely to serve as an intermediate workaround, here I
> take a simpler and shorter syntax from it (note it's allowed to have only
> one category being provided):
>     class=eth,mac=00:11:22:33:44:55:66
> 
> Also, old compatibility is kept. Users can still go on with using the PCI
> id to add a port (if that's enough for them). Meaning, this patch will not
> break anything.
> 
> This patch is basically based on the one from Ciara:
>     https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> October/339496.html
> 
> Cc: Loftus Ciara <ciara.loftus@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Kevin Traynor <ktraynor@redhat.com>
> Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>

Thanks for the work on this Yuanhan, it's a valuable feature/bug fix.

At this point I think this aligns to the DPDK model?

If so, and  if there are no other comments on this patchset then I plan to merge this to DPDK merge over the weekend.

Thanks
Ian

> ---
> 
> v3: - reuse eth helper functions from OVS, suggested by Ilya Maximets
> 
> v2: - added doc for the new devags syntax
>     - added error log for wrong mac format
> ---
>  Documentation/howto/dpdk.rst | 12 ++++++++
>  lib/netdev-dpdk.c            | 70 ++++++++++++++++++++++++++++++++++-----
> -----
>  2 files changed, 67 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
> index 2393c2f..e7cb7dd 100644
> --- a/Documentation/howto/dpdk.rst
> +++ b/Documentation/howto/dpdk.rst
> @@ -48,6 +48,18 @@ number of dpdk devices found in the log file::
>      $ ovs-vsctl add-port br0 dpdk-p1 -- set Interface dpdk-p1 type=dpdk \
>          options:dpdk-devargs=0000:01:00.1
> 
> +Some NICs (i.e. Mellanox ConnectX-3) have only one PCI address
> +associated with multiple ports. Using a PCI device like above won't
> +work. Instead, below usage is suggested::
> +
> +    $ ovs-vsctl add-port br0 dpdk-p0 -- set Interface dpdk-p0 type=dpdk \
> +        options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55:01"
> +    $ ovs-vsctl add-port br0 dpdk-p1 -- set Interface dpdk-p1 type=dpdk \
> +        options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55:02"
> +
> +Note: such syntax won't support hotplug. The hotplug is supposed to
> +work with future DPDK release, v18.05.
> +
>  After the DPDK ports get added to switch, a polling thread continuously
> polls  DPDK devices and consumes 100% of the core, as can be checked from
> ``top`` and  ``ps`` commands::
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 364f545..c4f0f5e
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1203,29 +1203,69 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
> }
> 
>  static dpdk_port_t
> +netdev_dpdk_get_port_by_mac(const char *mac_str) {
> +    dpdk_port_t port_id;
> +    struct eth_addr mac, port_mac;
> +
> +    if (!eth_addr_from_string(mac_str, &mac)) {
> +        VLOG_ERR("invalid mac: %s", mac_str);
> +        return DPDK_ETH_PORT_ID_INVALID;
> +    }
> +
> +    RTE_ETH_FOREACH_DEV (port_id) {
> +        struct ether_addr ea;
> +
> +        rte_eth_macaddr_get(port_id, &ea);
> +        memcpy(port_mac.ea, ea.addr_bytes, ETH_ADDR_LEN);
> +        if (eth_addr_equals(mac, port_mac)) {
> +            return port_id;
> +        }
> +    }
> +
> +    return DPDK_ETH_PORT_ID_INVALID;
> +}
> +
> +/*
> + * Normally, a PCI id is enough for identifying a specific DPDK port.
> + * However, for some NICs having multiple ports sharing the same PCI
> + * id, using PCI id won't work then.
> + *
> + * To fix that, here one more method is introduced: "class=eth,mac=$MAC".
> + *
> + * Note that the compatibility is fully kept: user can still use the
> + * PCI id for adding ports (when it's enough for them).
> + */
> +static dpdk_port_t
>  netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>                              const char *devargs, char **errp)  {
> -    /* Get the name up to the first comma. */
> -    char *name = xmemdup0(devargs, strcspn(devargs, ","));
> +    char *name;
>      dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> 
> -    if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> -            || !rte_eth_dev_is_valid_port(new_port_id)) {
> -        /* Device not found in DPDK, attempt to attach it */
> -        if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> -            /* Attach successful */
> -            dev->attached = true;
> -            VLOG_INFO("Device '%s' attached to DPDK", devargs);
> -        } else {
> -            /* Attach unsuccessful */
> -            new_port_id = DPDK_ETH_PORT_ID_INVALID;
> -            VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> -                          devargs);
> +    if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
> +        new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
> +    } else {
> +        name = xmemdup0(devargs, strcspn(devargs, ","));
> +        if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> +                || !rte_eth_dev_is_valid_port(new_port_id)) {
> +            /* Device not found in DPDK, attempt to attach it */
> +            if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> +                /* Attach successful */
> +                dev->attached = true;
> +                VLOG_INFO("Device '%s' attached to DPDK", devargs);
> +            } else {
> +                /* Attach unsuccessful */
> +                new_port_id = DPDK_ETH_PORT_ID_INVALID;
> +            }
>          }
> +        free(name);
> +    }
> +
> +    if (new_port_id == DPDK_ETH_PORT_ID_INVALID) {
> +        VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> + devargs);
>      }
> 
> -    free(name);
>      return new_port_id;
>  }
> 
> --
> 2.7.4
Yuanhan Liu Jan. 19, 2018, 2:47 p.m. | #2
On Fri, Jan 19, 2018 at 11:03:52AM +0000, Stokes, Ian wrote:
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > Sent: Wednesday, January 10, 2018 3:05 AM
> > To: dev@openvswitch.org
> > Cc: Stokes, Ian <ian.stokes@intel.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; Yuanhan Liu <yliu@fridaylinux.org>; Loftus, Ciara
> > <ciara.loftus@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Kevin
> > Traynor <ktraynor@redhat.com>
> > Subject: [PATCH v3] netdev-dpdk: fix port addition for ports sharing same
> > PCI id
> > 
> > Some NICs have only one PCI address associated with multiple ports. This
> > patch extends the dpdk-devargs option's format to cater for such devices.
> > 
> > To achieve that, this patch uses a new syntax that will be adapted and
> > implemented in future DPDK release (likely, v18.05):
> >     http://dpdk.org/ml/archives/dev/2017-December/084234.html
> > 
> > And since it's the DPDK duty to parse the (complete and full) syntax and
> > this patch is more likely to serve as an intermediate workaround, here I
> > take a simpler and shorter syntax from it (note it's allowed to have only
> > one category being provided):
> >     class=eth,mac=00:11:22:33:44:55:66
> > 
> > Also, old compatibility is kept. Users can still go on with using the PCI
> > id to add a port (if that's enough for them). Meaning, this patch will not
> > break anything.
> > 
> > This patch is basically based on the one from Ciara:
> >     https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> > October/339496.html
> > 
> > Cc: Loftus Ciara <ciara.loftus@intel.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Kevin Traynor <ktraynor@redhat.com>
> > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> 
> Thanks for the work on this Yuanhan, it's a valuable feature/bug fix.
> 
> At this point I think this aligns to the DPDK model?

Yes, I think so.

> 
> If so, and  if there are no other comments on this patchset then I plan to merge this to DPDK merge over the weekend.
> 

Thank you for taking care of it.

	--yliu
Ian Stokes Jan. 23, 2018, 1:34 p.m. | #3
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Friday, January 19, 2018 2:48 PM
> To: Stokes, Ian <ian.stokes@intel.com>
> Cc: dev@openvswitch.org; Shahaf Shuler <shahafs@mellanox.com>; Loftus,
> Ciara <ciara.loftus@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets
> (i.maximets@samsung.com) <i.maximets@samsung.com>; Olga Shern
> <olgas@mellanox.com>
> Subject: Re: [PATCH v3] netdev-dpdk: fix port addition for ports sharing
> same PCI id
> 
> On Fri, Jan 19, 2018 at 11:03:52AM +0000, Stokes, Ian wrote:
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > > Sent: Wednesday, January 10, 2018 3:05 AM
> > > To: dev@openvswitch.org
> > > Cc: Stokes, Ian <ian.stokes@intel.com>; Shahaf Shuler
> > > <shahafs@mellanox.com>; Yuanhan Liu <yliu@fridaylinux.org>; Loftus,
> > > Ciara <ciara.loftus@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Kevin Traynor <ktraynor@redhat.com>
> > > Subject: [PATCH v3] netdev-dpdk: fix port addition for ports sharing
> > > same PCI id
> > >
> > > Some NICs have only one PCI address associated with multiple ports.
> > > This patch extends the dpdk-devargs option's format to cater for such
> devices.
> > >
> > > To achieve that, this patch uses a new syntax that will be adapted
> > > and implemented in future DPDK release (likely, v18.05):
> > >     http://dpdk.org/ml/archives/dev/2017-December/084234.html
> > >
> > > And since it's the DPDK duty to parse the (complete and full) syntax
> > > and this patch is more likely to serve as an intermediate
> > > workaround, here I take a simpler and shorter syntax from it (note
> > > it's allowed to have only one category being provided):
> > >     class=eth,mac=00:11:22:33:44:55:66
> > >
> > > Also, old compatibility is kept. Users can still go on with using
> > > the PCI id to add a port (if that's enough for them). Meaning, this
> > > patch will not break anything.
> > >
> > > This patch is basically based on the one from Ciara:
> > >     https://mail.openvswitch.org/pipermail/ovs-dev/2017-
> > > October/339496.html
> > >
> > > Cc: Loftus Ciara <ciara.loftus@intel.com>
> > > Cc: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: Kevin Traynor <ktraynor@redhat.com>
> > > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> >
> > Thanks for the work on this Yuanhan, it's a valuable feature/bug fix.
> >
> > At this point I think this aligns to the DPDK model?
> 
> Yes, I think so.
> 
> >
> > If so, and  if there are no other comments on this patchset then I plan
> to merge this to DPDK merge over the weekend.
> >
> 
> Thank you for taking care of it.

Thanks for all the work on this, I've pushed this to DPDK_MERGE and will also push it to DPDK_MERGE_2_9 for it to be part of the 2.9 release.

Ian
> 
> 	--yliu

Patch

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 2393c2f..e7cb7dd 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -48,6 +48,18 @@  number of dpdk devices found in the log file::
     $ ovs-vsctl add-port br0 dpdk-p1 -- set Interface dpdk-p1 type=dpdk \
         options:dpdk-devargs=0000:01:00.1
 
+Some NICs (i.e. Mellanox ConnectX-3) have only one PCI address associated
+with multiple ports. Using a PCI device like above won't work. Instead, below
+usage is suggested::
+
+    $ ovs-vsctl add-port br0 dpdk-p0 -- set Interface dpdk-p0 type=dpdk \
+        options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55:01"
+    $ ovs-vsctl add-port br0 dpdk-p1 -- set Interface dpdk-p1 type=dpdk \
+        options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55:02"
+
+Note: such syntax won't support hotplug. The hotplug is supposed to work with
+future DPDK release, v18.05.
+
 After the DPDK ports get added to switch, a polling thread continuously polls
 DPDK devices and consumes 100% of the core, as can be checked from ``top`` and
 ``ps`` commands::
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 364f545..c4f0f5e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1203,29 +1203,69 @@  netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
 }
 
 static dpdk_port_t
+netdev_dpdk_get_port_by_mac(const char *mac_str)
+{
+    dpdk_port_t port_id;
+    struct eth_addr mac, port_mac;
+
+    if (!eth_addr_from_string(mac_str, &mac)) {
+        VLOG_ERR("invalid mac: %s", mac_str);
+        return DPDK_ETH_PORT_ID_INVALID;
+    }
+
+    RTE_ETH_FOREACH_DEV (port_id) {
+        struct ether_addr ea;
+
+        rte_eth_macaddr_get(port_id, &ea);
+        memcpy(port_mac.ea, ea.addr_bytes, ETH_ADDR_LEN);
+        if (eth_addr_equals(mac, port_mac)) {
+            return port_id;
+        }
+    }
+
+    return DPDK_ETH_PORT_ID_INVALID;
+}
+
+/*
+ * Normally, a PCI id is enough for identifying a specific DPDK port.
+ * However, for some NICs having multiple ports sharing the same PCI
+ * id, using PCI id won't work then.
+ *
+ * To fix that, here one more method is introduced: "class=eth,mac=$MAC".
+ *
+ * Note that the compatibility is fully kept: user can still use the
+ * PCI id for adding ports (when it's enough for them).
+ */
+static dpdk_port_t
 netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
                             const char *devargs, char **errp)
 {
-    /* Get the name up to the first comma. */
-    char *name = xmemdup0(devargs, strcspn(devargs, ","));
+    char *name;
     dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
 
-    if (rte_eth_dev_get_port_by_name(name, &new_port_id)
-            || !rte_eth_dev_is_valid_port(new_port_id)) {
-        /* Device not found in DPDK, attempt to attach it */
-        if (!rte_eth_dev_attach(devargs, &new_port_id)) {
-            /* Attach successful */
-            dev->attached = true;
-            VLOG_INFO("Device '%s' attached to DPDK", devargs);
-        } else {
-            /* Attach unsuccessful */
-            new_port_id = DPDK_ETH_PORT_ID_INVALID;
-            VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
-                          devargs);
+    if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
+        new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
+    } else {
+        name = xmemdup0(devargs, strcspn(devargs, ","));
+        if (rte_eth_dev_get_port_by_name(name, &new_port_id)
+                || !rte_eth_dev_is_valid_port(new_port_id)) {
+            /* Device not found in DPDK, attempt to attach it */
+            if (!rte_eth_dev_attach(devargs, &new_port_id)) {
+                /* Attach successful */
+                dev->attached = true;
+                VLOG_INFO("Device '%s' attached to DPDK", devargs);
+            } else {
+                /* Attach unsuccessful */
+                new_port_id = DPDK_ETH_PORT_ID_INVALID;
+            }
         }
+        free(name);
+    }
+
+    if (new_port_id == DPDK_ETH_PORT_ID_INVALID) {
+        VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
     }
 
-    free(name);
     return new_port_id;
 }