[ovs-dev,RFC] netdev-dpdk: Allow specification of index for PCI devices

Message ID 1507298214-17028-1-git-send-email-ciara.loftus@intel.com
State Superseded
Headers show
Series
  • [ovs-dev,RFC] netdev-dpdk: Allow specification of index for PCI devices
Related show

Commit Message

Ciara Loftus Oct. 6, 2017, 1:56 p.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. Whereas before only one of N ports associated with one PCI
address could be added, now N can.

This patch allows for the following use of the dpdk-devargs option:

ovs-vsctl set Interface myport options:dpdk-devargs=0000:06:00.0,X

Where X is an unsigned integer representing one of multiple ports
associated with the PCI address provided.

This patch has not been tested.

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 lib/netdev-dpdk.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 16 deletions(-)

Comments

devendra rawat Oct. 10, 2017, 12:59 p.m. | #1
Hi Ciara,

On Fri, Oct 6, 2017 at 7:26 PM, Ciara Loftus <ciara.loftus@intel.com> wrote:

> 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. Whereas before only one of N ports associated with one PCI
> address could be added, now N can.
>
> This patch allows for the following use of the dpdk-devargs option:
>
> ovs-vsctl set Interface myport options:dpdk-devargs=0000:06:00.0,X
>
> Where X is an unsigned integer representing one of multiple ports
> associated with the PCI address provided.
>
> This patch has not been tested.
>

I tested this patch on top of OVS v2.8.1 and DPDK v17.08. I used Mellanox
ConnectX-3 pro NIC for testing, this NIC provides two 10G ports
that share a single PCI address.

The patch is working fine, I was able to add both the 10G ports to OVS
bridge by specifying the port no. (0 or 1) in the dpdk-devargs.

# ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
options:dpdk-devargs=0002:01:00.0,0
# ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
options:dpdk-devargs=0002:01:00.0,1

the port no. (0 and 1) are bound to actual physical ports, i.e if I keep on
adding and deleting port no. 0 to bridge br0 multiple times,
every time I add the port back to br0, the same physical port is added to
bridge br0.

Thanks,
Devendra


>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>  lib/netdev-dpdk.c | 79 ++++++++++++++++++++++++++++++
> ++++++++++++++-----------
>  1 file changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index c60f46f..c3562a3 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1214,30 +1214,77 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
>  }
>
>  static dpdk_port_t
> +process_devargs_index(char *name, int idx)
> +{
> +    int i = 0;
> +    struct rte_eth_dev_info dev_info;
> +    char pci_addr[PCI_PRI_STR_SIZE];
> +    dpdk_port_t offset_port_id = DPDK_ETH_PORT_ID_INVALID;
> +
> +    for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +        if (!rte_eth_dev_is_valid_port(i)) {
> +            continue;
> +        }
> +        rte_eth_dev_info_get(i, &dev_info);
> +        rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> +                            sizeof(pci_addr));
> +        if (!strcmp(pci_addr, name)) {
> +            offset_port_id = i + idx;
> +            if (rte_eth_dev_is_valid_port(offset_port_id)) {
> +                rte_eth_dev_info_get(offset_port_id, &dev_info);
> +                rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> +                                    sizeof(pci_addr));
> +                if (!strcmp(pci_addr, name)) {
> +                    return offset_port_id;
> +                } else {
> +                    break;
> +                }
> +            } else {
> +                break;
> +            }
> +        }
> +    }
> +
> +    VLOG_INFO("Invalid PCI offset %i for device %s", idx, name);
> +    return DPDK_ETH_PORT_ID_INVALID;
> +}
> +
> +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 *devargs_copy = xmemdup0((devargs), strlen(devargs));
> +    char *name, *idx_str;
> +    unsigned idx;
>      dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
>
> -    if (!rte_eth_dev_count()
> -            || 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);
> +    name = strtok_r(devargs_copy, ",", &devargs_copy);
> +    idx_str = strtok_r(devargs_copy, ",", &devargs_copy);
> +
> +    if (idx_str) {
> +        idx = atoi(idx_str);
> +        new_port_id = process_devargs_index(name, idx);
> +    } else {
> +        if (!rte_eth_dev_count()
> +                || 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);
> +    }
> +
>      return new_port_id;
>  }
>
> --
> 2.7.5
>
>
Stokes, Ian Oct. 15, 2017, 4:31 p.m. | #2
Hi Ciara, thanks for working on this patch. A few comments inline.

> 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.
> Whereas before only one of N ports associated with one PCI address could
> be added, now N can.
> 
> This patch allows for the following use of the dpdk-devargs option:
> 
> ovs-vsctl set Interface myport options:dpdk-devargs=0000:06:00.0,X
> 
> Where X is an unsigned integer representing one of multiple ports
> associated with the PCI address provided.
> 
> This patch has not been tested.

Have you considered the hotplug usecase?

I think attaching in this case will be ok but OVS provides a mechanism to detach via appctl as well using the pci address (see command below.

$ ovs-appctl netdev-dpdk/detach 0000:01:00.0

I think this will have to be modified also to take into account the index of the port otherwise we hit the same situation again.

It's probably something that will need to be tested with this patch going forward.

Ian
> 
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>  lib/netdev-dpdk.c | 79 ++++++++++++++++++++++++++++++++++++++++++++------
> -----
>  1 file changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index c60f46f..c3562a3
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1214,30 +1214,77 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
> }
> 
>  static dpdk_port_t
> +process_devargs_index(char *name, int idx) {
> +    int i = 0;
> +    struct rte_eth_dev_info dev_info;
> +    char pci_addr[PCI_PRI_STR_SIZE];
> +    dpdk_port_t offset_port_id = DPDK_ETH_PORT_ID_INVALID;
> +
> +    for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +        if (!rte_eth_dev_is_valid_port(i)) {
> +            continue;
> +        }
> +        rte_eth_dev_info_get(i, &dev_info);
> +        rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> +                            sizeof(pci_addr));
> +        if (!strcmp(pci_addr, name)) {
> +            offset_port_id = i + idx;
> +            if (rte_eth_dev_is_valid_port(offset_port_id)) {
> +                rte_eth_dev_info_get(offset_port_id, &dev_info);
> +                rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> +                                    sizeof(pci_addr));
> +                if (!strcmp(pci_addr, name)) {
> +                    return offset_port_id;
> +                } else {
> +                    break;
> +                }
> +            } else {
> +                break;
> +            }
> +        }
> +    }
> +
> +    VLOG_INFO("Invalid PCI offset %i for device %s", idx, name);
> +    return DPDK_ETH_PORT_ID_INVALID;
> +}
> +
> +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 *devargs_copy = xmemdup0((devargs), strlen(devargs));
> +    char *name, *idx_str;
> +    unsigned idx;
>      dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> 
> -    if (!rte_eth_dev_count()
> -            || 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);
> +    name = strtok_r(devargs_copy, ",", &devargs_copy);
> +    idx_str = strtok_r(devargs_copy, ",", &devargs_copy);
> +
> +    if (idx_str) {
> +        idx = atoi(idx_str);
> +        new_port_id = process_devargs_index(name, idx);
> +    } else {
> +        if (!rte_eth_dev_count()
> +                || 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);
> +    }
> +
>      return new_port_id;
>  }
> 
> --
> 2.7.5
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ciara Loftus Oct. 16, 2017, 12:38 p.m. | #3
> 
> Hi Ciara, thanks for working on this patch. A few comments inline.

Thanks for your review Ian.

> 
> > 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.
> > Whereas before only one of N ports associated with one PCI address could
> > be added, now N can.
> >
> > This patch allows for the following use of the dpdk-devargs option:
> >
> > ovs-vsctl set Interface myport options:dpdk-devargs=0000:06:00.0,X
> >
> > Where X is an unsigned integer representing one of multiple ports
> > associated with the PCI address provided.
> >
> > This patch has not been tested.
> 
> Have you considered the hotplug usecase?
> 
> I think attaching in this case will be ok but OVS provides a mechanism to
> detach via appctl as well using the pci address (see command below.
> 
> $ ovs-appctl netdev-dpdk/detach 0000:01:00.0
> 
> I think this will have to be modified also to take into account the index of the
> port otherwise we hit the same situation again.
> 
> It's probably something that will need to be tested with this patch going
> forward.

It should be easy to reuse the process_devargs_index function for this case. I'll include this in a v2.

Thanks,
Ciara

> 
> Ian
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> >  lib/netdev-dpdk.c | 79
> ++++++++++++++++++++++++++++++++++++++++++++------
> > -----
> >  1 file changed, 63 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index c60f46f..c3562a3
> > 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1214,30 +1214,77 @@
> netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
> > }
> >
> >  static dpdk_port_t
> > +process_devargs_index(char *name, int idx) {
> > +    int i = 0;
> > +    struct rte_eth_dev_info dev_info;
> > +    char pci_addr[PCI_PRI_STR_SIZE];
> > +    dpdk_port_t offset_port_id = DPDK_ETH_PORT_ID_INVALID;
> > +
> > +    for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > +        if (!rte_eth_dev_is_valid_port(i)) {
> > +            continue;
> > +        }
> > +        rte_eth_dev_info_get(i, &dev_info);
> > +        rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> > +                            sizeof(pci_addr));
> > +        if (!strcmp(pci_addr, name)) {
> > +            offset_port_id = i + idx;
> > +            if (rte_eth_dev_is_valid_port(offset_port_id)) {
> > +                rte_eth_dev_info_get(offset_port_id, &dev_info);
> > +                rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
> > +                                    sizeof(pci_addr));
> > +                if (!strcmp(pci_addr, name)) {
> > +                    return offset_port_id;
> > +                } else {
> > +                    break;
> > +                }
> > +            } else {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    VLOG_INFO("Invalid PCI offset %i for device %s", idx, name);
> > +    return DPDK_ETH_PORT_ID_INVALID;
> > +}
> > +
> > +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 *devargs_copy = xmemdup0((devargs), strlen(devargs));
> > +    char *name, *idx_str;
> > +    unsigned idx;
> >      dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> >
> > -    if (!rte_eth_dev_count()
> > -            || 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);
> > +    name = strtok_r(devargs_copy, ",", &devargs_copy);
> > +    idx_str = strtok_r(devargs_copy, ",", &devargs_copy);
> > +
> > +    if (idx_str) {
> > +        idx = atoi(idx_str);
> > +        new_port_id = process_devargs_index(name, idx);
> > +    } else {
> > +        if (!rte_eth_dev_count()
> > +                || 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);
> > +    }
> > +
> >      return new_port_id;
> >  }
> >
> > --
> > 2.7.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c60f46f..c3562a3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1214,30 +1214,77 @@  netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
 }
 
 static dpdk_port_t
+process_devargs_index(char *name, int idx)
+{
+    int i = 0;
+    struct rte_eth_dev_info dev_info;
+    char pci_addr[PCI_PRI_STR_SIZE];
+    dpdk_port_t offset_port_id = DPDK_ETH_PORT_ID_INVALID;
+
+    for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+        if (!rte_eth_dev_is_valid_port(i)) {
+            continue;
+        }
+        rte_eth_dev_info_get(i, &dev_info);
+        rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
+                            sizeof(pci_addr));
+        if (!strcmp(pci_addr, name)) {
+            offset_port_id = i + idx;
+            if (rte_eth_dev_is_valid_port(offset_port_id)) {
+                rte_eth_dev_info_get(offset_port_id, &dev_info);
+                rte_pci_device_name(&dev_info.pci_dev->addr, pci_addr,
+                                    sizeof(pci_addr));
+                if (!strcmp(pci_addr, name)) {
+                    return offset_port_id;
+                } else {
+                    break;
+                }
+            } else {
+                break;
+            }
+        }
+    }
+
+    VLOG_INFO("Invalid PCI offset %i for device %s", idx, name);
+    return DPDK_ETH_PORT_ID_INVALID;
+}
+
+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 *devargs_copy = xmemdup0((devargs), strlen(devargs));
+    char *name, *idx_str;
+    unsigned idx;
     dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
 
-    if (!rte_eth_dev_count()
-            || 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);
+    name = strtok_r(devargs_copy, ",", &devargs_copy);
+    idx_str = strtok_r(devargs_copy, ",", &devargs_copy);
+
+    if (idx_str) {
+        idx = atoi(idx_str);
+        new_port_id = process_devargs_index(name, idx);
+    } else {
+        if (!rte_eth_dev_count()
+                || 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);
+    }
+
     return new_port_id;
 }