Not a full review.
There are few style issues. Also same code snippets appears 3 times
in a code, so, it's better to make a function from them.
I prepared the incremental for those changes. (Compile tested only)
Please, take a look:
----
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 7ce067d0a..53ef55865 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1231,7 +1231,7 @@ netdev_dpdk_get_num_ports(struct rte_device *device)
LIST_FOR_EACH (dev, list_node, &dpdk_list) {
if (rte_eth_devices[dev->port_id].device == device
- && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
+ && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
count++;
}
}
@@ -1676,6 +1676,23 @@ netdev_dpdk_get_port_by_mac(const char *mac_str)
return DPDK_ETH_PORT_ID_INVALID;
}
+/* Return the first DPDK port id matching the devargs pattern. */
+static dpdk_port_t
+netdev_dpdk_get_port_by_devargs(const char *devargs)
+ OVS_REQUIRES(dpdk_mutex)
+{
+ dpdk_port_t port_id;
+ struct rte_dev_iterator iterator;
+
+ RTE_ETH_FOREACH_MATCHING_DEV (port_id, devargs, &iterator) {
+ /* If a break is done - must call rte_eth_iterator_cleanup. */
+ rte_eth_iterator_cleanup(&iterator);
+ break;
+ }
+
+ return port_id;
+}
+
/*
* Normally, a PCI id (optionally followed by a representor number)
* is enough for identifying a specific DPDK port.
@@ -1692,29 +1709,18 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
const char *devargs, char **errp)
OVS_REQUIRES(dpdk_mutex)
{
- struct rte_dev_iterator iterator;
dpdk_port_t new_port_id;
if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
} else {
- /* Return the first DPDK port id matching the devargs pattern. */
- RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, devargs, &iterator) {
- /* If a break is done - must call rte_eth_iterator_cleanup. */
- rte_eth_iterator_cleanup(&iterator);
- break;
- }
+ new_port_id = netdev_dpdk_get_port_by_devargs(devargs);
if (!rte_eth_dev_is_valid_port(new_port_id)) {
/* Device not found in DPDK, attempt to attach it */
if (rte_dev_probe(devargs)) {
new_port_id = DPDK_ETH_PORT_ID_INVALID;
} else {
- RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, \
- devargs, &iterator) {
- /* If a break is done - call rte_eth_iterator_cleanup. */
- rte_eth_iterator_cleanup(&iterator);
- break;
- }
+ new_port_id = netdev_dpdk_get_port_by_devargs(devargs);
if (rte_eth_dev_is_valid_port(new_port_id)) {
/* Attach successful */
dev->attached = true;
@@ -1725,7 +1731,7 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
}
}
}
- }
+ }
if (new_port_id == DPDK_ETH_PORT_ID_INVALID) {
VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
@@ -3295,18 +3301,13 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
dpdk_port_t port_id;
struct netdev_dpdk *dev;
struct rte_device *rte_dev;
- struct rte_dev_iterator iterator;
struct ds used_interfaces = DS_EMPTY_INITIALIZER;
bool used = false;
ovs_mutex_lock(&dpdk_mutex);
- RTE_ETH_FOREACH_MATCHING_DEV (port_id, argv[1], &iterator) {
- /* If a break is done - must call rte_eth_iterator_cleanup. */
- rte_eth_iterator_cleanup(&iterator);
- break;
- }
- if (port_id == DPDK_ETH_PORT_ID_INVALID) {
+ port_id = netdev_dpdk_get_port_by_devargs(argv[1]);
+ if (!rte_eth_dev_is_valid_port(port_id)) {
response = xasprintf("Device '%s' not found in DPDK", argv[1]);
goto error;
}
@@ -3319,7 +3320,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
LIST_FOR_EACH (dev, list_node, &dpdk_list) {
/* FIXME: avoid direct access to DPDK array rte_eth_devices. */
if (rte_eth_devices[dev->port_id].device == rte_dev
- && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
+ && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
used = true;
ds_put_format(&used_interfaces, " %s",
netdev_get_name(&dev->up));
@@ -3341,7 +3342,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
OVS_UNUSED,
}
response = xasprintf("All devices shared with device '%s' "
- "have been detached", argv[1]);
+ "have been detached", argv[1]);
ovs_mutex_unlock(&dpdk_mutex);
unixctl_command_reply(conn, response);
----
Best regards, Ilya Maximets.
On 17.01.2019 18:58, Ophir Munk wrote:
> Dpdk port representors were introduced in dpdk versions 18.xx.
> Prior to port representors there was a one-to-one relationship
> between an rte device (e.g. PCI bus) and an eth device (referenced as
> dpdk port id in OVS). With port representors the relationship becomes
> one-to-many rte device to eth devices.
> For example in [3] there are two devices (representors) using the same
> PCI physical address 0000:08:00.0: "0000:08:00.0,representor=[3]" and
> "0000:08:00.0,representor=[5]".
> This commit handles the new one-to-many relationship. For example,
> when one of the device port representors in [3] is closed - the PCI bus
> cannot be detached until the other device port representor is closed as
> well. OVS remains backward compatible by supporting dpdk legacy PCI
> ports which do not include port representors.
> Dpdk port representors related commits are listed in [1]. Dpdk port
> representors documentation appears in [2]. A sample configuration
> which uses two representors ports (the output of "ovs-vsctl show"
> command) is shown in [3].
>
> [1]
> e0cb96204b71 ("net/i40e: add support for representor ports")
> cf80ba6e2038 ("net/ixgbe: add support for representor ports")
> 26c08b979d26 ("net/mlx5: add port representor awareness")
>
> [2]
> https://doc.dpdk.org/guides-18.11/prog_guide/switch_representation.html
>
> [3]
> Bridge "ovs_br0"
> Port "ovs_br0"
> Interface "ovs_br0"
> type: internal
> Port "port-rep3"
> Interface "port-rep3"
> type: dpdk
> options: {dpdk-devargs="0000:08:00.0,representor=[3]"}
> Port "port-rep5"
> Interface "port-rep5"
> type: dpdk
> options: {dpdk-devargs="0000:08:00.0,representor=[5]"}
> ovs_version: "2.10.90"
>
> Signed-off-by: Ophir Munk <[email protected]>
> Co-authored-by: Ilya Maximets <[email protected]>
> ---
> v1 (hwol branch):
> 1. rebase on top of Kevin's patch
> dpdk: Update to use DPDK 18.11.[ovs-dev,v7,dpdk-latest,1/1] dpdk: Update to
> use DPDK 18.11.
> https://patchwork.ozlabs.org/patch/1005535/
> 2. skipping count of sibling ports in case the sibling port state is
> RTE_ETH_DEV_UNUSED
>
> v2 (switching to master branch):
> 1. Update based on review comments:
> https://patchwork.ozlabs.org/patch/1011457/#2051417
> 2. Send patch on master branch
>
> v3:
> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices array
>
> v4:
> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
> 2. Do not probe unconditionally during add-port. Instead see if a device has
> already
> been probed, and if so skip the rte_dev_probe(devargs)
>
> v5:
> 1. Updates following v4 reviews
> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported
> (for PMDs not supporting representors)
>
> v6:
> 1. Updates following v4 & v5 reviews
> 2. Eliminate double checking of dup devices using the same devargs
> 3. Fix assignment of dev->attached=true to occur only when the device is
> probed
> 4. Update netdev_dpdk_detach to print all devices using the same rte_device
> 5. Documentation will be updated in a follow up commit
> 6. Adding Ilya Maximets as co-author
>
> lib/netdev-dpdk.c | 144
> ++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 112 insertions(+), 32 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 320422b..7ce067d 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1218,6 +1218,26 @@ dpdk_dev_parse_name(const char dev_name[], const char
> prefix[],
> }
> }
>
> +/* Get the number of OVS interfaces which have the same DPDK
> + * rte device (e.g. same pci bus address).
> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> + */
> +static int
> +netdev_dpdk_get_num_ports(struct rte_device *device)
> + OVS_REQUIRES(dpdk_mutex)
> +{
> + struct netdev_dpdk *dev;
> + int count = 0;
> +
> + LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> + if (rte_eth_devices[dev->port_id].device == device
> + && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
> + count++;
> + }
> + }
> + return count;
> +}
> +
> static int
> vhost_common_construct(struct netdev *netdev)
> OVS_REQUIRES(dpdk_mutex)
> @@ -1353,7 +1373,9 @@ static void
> netdev_dpdk_destruct(struct netdev *netdev)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> - struct rte_eth_dev_info dev_info;
> + struct rte_device *rte_dev;
> + struct rte_eth_dev *eth_dev;
> + bool remove_on_close;
>
> ovs_mutex_lock(&dpdk_mutex);
>
> @@ -1361,12 +1383,34 @@ netdev_dpdk_destruct(struct netdev *netdev)
> dev->started = false;
>
> if (dev->attached) {
> + /* Retrieve eth device data before closing it.
> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> + */
> + eth_dev = &rte_eth_devices[dev->port_id];
> + remove_on_close =
> + eth_dev->data &&
> + (eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE);
> + rte_dev = eth_dev->device;
> +
> + /* Remove the eth device. */
> rte_eth_dev_close(dev->port_id);
> - rte_eth_dev_info_get(dev->port_id, &dev_info);
> - if (dev_info.device && !rte_dev_remove(dev_info.device)) {
> - VLOG_INFO("Device '%s' has been detached", dev->devargs);
> +
> + /* Remove this rte device and all its eth devices if flag
> + * RTE_ETH_DEV_CLOSE_REMOVE is not supported (which means
> representors
> + * are not supported), or if all the eth devices belonging to the rte
> + * device are closed.
> + */
> + if (!remove_on_close || !netdev_dpdk_get_num_ports(rte_dev)) {
> + if (rte_dev_remove(rte_dev) < 0) {
> + VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> + } else {
> + /* Device was closed and detached. */
> + VLOG_INFO("Device '%s' has been removed and detached",
> + dev->devargs);
> + }
> } else {
> - VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> + /* Device was only closed. rte_dev_remove() was not called. */
> + VLOG_INFO("Device '%s' has been removed", dev->devargs);
> }
> }
>
> @@ -1633,7 +1677,8 @@ netdev_dpdk_get_port_by_mac(const char *mac_str)
> }
>
> /*
> - * Normally, a PCI id is enough for identifying a specific DPDK port.
> + * Normally, a PCI id (optionally followed by a representor number)
> + * 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.
> *
> @@ -1645,29 +1690,42 @@ netdev_dpdk_get_port_by_mac(const char *mac_str)
> static dpdk_port_t
> netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> const char *devargs, char **errp)
> + OVS_REQUIRES(dpdk_mutex)
> {
> - char *name;
> - dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> + struct rte_dev_iterator iterator;
> + dpdk_port_t new_port_id;
>
> 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)) {
> + /* Return the first DPDK port id matching the devargs pattern. */
> + RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, devargs, &iterator) {
> + /* If a break is done - must call rte_eth_iterator_cleanup. */
> + rte_eth_iterator_cleanup(&iterator);
> + break;
> + }
> + if (!rte_eth_dev_is_valid_port(new_port_id)) {
> /* Device not found in DPDK, attempt to attach it */
> - if (!rte_dev_probe(devargs)
> - && !rte_eth_dev_get_port_by_name(name, &new_port_id)) {
> - /* Attach successful */
> - dev->attached = true;
> - VLOG_INFO("Device '%s' attached to DPDK", devargs);
> - } else {
> - /* Attach unsuccessful */
> + if (rte_dev_probe(devargs)) {
> new_port_id = DPDK_ETH_PORT_ID_INVALID;
> + } else {
> + RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, \
> + devargs, &iterator) {
> + /* If a break is done - call rte_eth_iterator_cleanup. */
> + rte_eth_iterator_cleanup(&iterator);
> + break;
> + }
> + if (rte_eth_dev_is_valid_port(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);
> @@ -1761,7 +1819,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
> dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
> if (dup_dev) {
> VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
> - "which is already in use by '%s'",
> + "which is already in use by '%s'",
> netdev_get_name(netdev), new_devargs,
> netdev_get_name(&dup_dev->up));
> err = EADDRINUSE;
> @@ -3236,32 +3294,54 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
> char *response;
> dpdk_port_t port_id;
> struct netdev_dpdk *dev;
> - struct rte_eth_dev_info dev_info;
> + struct rte_device *rte_dev;
> + struct rte_dev_iterator iterator;
> + struct ds used_interfaces = DS_EMPTY_INITIALIZER;
> + bool used = false;
>
> ovs_mutex_lock(&dpdk_mutex);
>
> - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) {
> + RTE_ETH_FOREACH_MATCHING_DEV (port_id, argv[1], &iterator) {
> + /* If a break is done - must call rte_eth_iterator_cleanup. */
> + rte_eth_iterator_cleanup(&iterator);
> + break;
> + }
> + if (port_id == DPDK_ETH_PORT_ID_INVALID) {
> response = xasprintf("Device '%s' not found in DPDK", argv[1]);
> goto error;
> }
>
> - 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));
> + rte_dev = rte_eth_devices[port_id].device;
> + ds_put_format(&used_interfaces,
> + "Device '%s' is being used by the following interfaces:",
> + argv[1]);
> +
> + LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> + /* FIXME: avoid direct access to DPDK array rte_eth_devices. */
> + if (rte_eth_devices[dev->port_id].device == rte_dev
> + && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
> + used = true;
> + ds_put_format(&used_interfaces, " %s",
> + netdev_get_name(&dev->up));
> + }
> + }
> +
> + if (used) {
> + ds_put_cstr(&used_interfaces, ". Remove them before detaching.");
> + response = ds_steal_cstr(&used_interfaces);
> + ds_destroy(&used_interfaces);
> goto error;
> }
> + ds_destroy(&used_interfaces);
>
> rte_eth_dev_close(port_id);
> -
> - rte_eth_dev_info_get(port_id, &dev_info);
> - if (!dev_info.device || rte_dev_remove(dev_info.device)) {
> + if (rte_dev_remove(rte_dev) < 0) {
> response = xasprintf("Device '%s' can not be detached", argv[1]);
> goto error;
> }
>
> - response = xasprintf("Device '%s' has been detached", argv[1]);
> + response = xasprintf("All devices shared with device '%s' "
> + "have been detached", argv[1]);
>
> ovs_mutex_unlock(&dpdk_mutex);
> unixctl_command_reply(conn, response);
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
