| 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 | expand |
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 > >
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
> > 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
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; }
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(-)