[RFC 0/4] ethdev: rework config restore

[RFC 0/4] ethdev: rework config restore

[Dariusz Sosnowski]

Hi all,

We have been working on optimizing the latency of calls to rte_eth_dev_start(),
on ports spawned by mlx5 PMD. Most of the work requires changes in the implementation of
.dev_start() PMD callback, but I also wanted to start a discussion regarding
configuration restore.

rte_eth_dev_start() does a few things on top of calling .dev_start() callback:

- Before calling it:
    - eth_dev_mac_restore() - if device supports RTE_ETH_DEV_NOLIVE_MAC_ADDR;
- After calling it:
    - eth_dev_mac_restore() - if device does not support RTE_ETH_DEV_NOLIVE_MAC_ADDR;
    - restore promiscuous config
    - restore all multicast config

eth_dev_mac_restore() iterates over all known MAC addresses -
stored in rte_eth_dev_data.mac_addrs array - and calls
.mac_addr_set() and .mac_addr_add() callbacks to apply these MAC addresses.

Promiscuous config restore checks if promiscuous mode is enabled or not,
and calls .promiscuous_enable() or .promiscuous_disable() callback.

All multicast config restore checks if all multicast mode is enabled or not,
and calls .allmulticast_enable() or .allmulticast_disable() callback.

Callbacks are called directly in all of these cases, to bypass the checks
for applying the same configuration, which exist in relevant APIs.
Checks are bypassed to force drivers to reapply the configuration.

Let's consider what happens in the following sequence of API calls.

1. rte_eth_dev_configure()
2. rte_eth_tx_queue_setup()
3. rte_eth_rx_queue_setup()
4. rte_eth_promiscuous_enable()
    - Call dev->dev_ops->promiscuous_enable()
    - Stores promiscuous state in dev->data->promiscuous
5. rte_eth_allmulticast_enable()
    - Call dev->dev_ops->allmulticast_enable()
    - Stores allmulticast state in dev->data->allmulticast
6. rte_eth_dev_start()
    - Call dev->dev_ops->dev_start()
    - Call dev->dev_ops->mac_addr_set() - apply default MAC address
    - Call dev->dev_ops->promiscuous_enable()
    - Call dev->dev_ops->allmulticast_enable()

Even though all configuration is available in dev->data after step 5,
library forces reapplying this configuration in step 6.

In mlx5 PMD case all relevant callbacks require communication with the kernel driver,
to configure the device (mlx5 PMD must create/destroy new kernel flow rules and/or
change netdev config).

mlx5 PMD handles applying all configuration in .dev_start(), so the following forced callbacks
force additional communication with the kernel. The same configuration is applied multiple times.

As an optimization, mlx5 PMD could check if a given configuration was applied,
but this would duplicate the functionality of the library
(for example rte_eth_promiscuous_enable() does not call the driver
if dev->data->promiscuous is set).

Question: Since all of the configuration is available before .dev_start() callback is called,
why ethdev library does not expect .dev_start() to take this configuration into account?
In other words, why library has to reapply the configuration?

I could not find any particular reason why configuration restore exists
as part of the process (it was in the initial DPDK commit).

The patches included in this RFC, propose a mechanism which would help with managing which drivers
rely on forceful configuration restore.
Drivers could advertise if forceful configuration restore is needed through
`RTE_ETH_DEV_*_FORCE_RESTORE` device flag. If this flag is set, then the driver in question
requires ethdev to forcefully restore configuration.

This way, if we would conclude that it makes sense for .dev_start() to handle all
starting configuration aspects, we could track which drivers still rely on configuration restore.

Dariusz Sosnowski (4):
  ethdev: rework config restore
  ethdev: omit promiscuous config restore if not required
  ethdev: omit all multicast config restore if not required
  ethdev: omit MAC address restore if not required

 lib/ethdev/rte_ethdev.c | 39 ++++++++++++++++++++++++++++++++++-----
 lib/ethdev/rte_ethdev.h | 18 ++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

--
2.39.5

[RFC 1/4] ethdev: rework config restore

[Dariusz Sosnowski]

Extract promiscuous and all multicast configuration restore
to separate functions.
This change will allow easier integration of disabling
these procedures for supporting PMDs in follow up commits.

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
---
 lib/ethdev/rte_ethdev.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f1c658f49e..362a1883f0 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1648,14 +1648,10 @@ eth_dev_mac_restore(struct rte_eth_dev *dev,
 }
 
 static int
-eth_dev_config_restore(struct rte_eth_dev *dev,
-		struct rte_eth_dev_info *dev_info, uint16_t port_id)
+eth_dev_promiscuous_restore(struct rte_eth_dev *dev, uint16_t port_id)
 {
 	int ret;
 
-	if (!(*dev_info->dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR))
-		eth_dev_mac_restore(dev, dev_info);
-
 	/* replay promiscuous configuration */
 	/*
 	 * use callbacks directly since we don't need port_id check and
@@ -1683,6 +1679,14 @@ eth_dev_config_restore(struct rte_eth_dev *dev,
 		}
 	}
 
+	return 0;
+}
+
+static int
+eth_dev_allmulticast_restore(struct rte_eth_dev *dev, uint16_t port_id)
+{
+	int ret;
+
 	/* replay all multicast configuration */
 	/*
 	 * use callbacks directly since we don't need port_id check and
@@ -1713,6 +1717,26 @@ eth_dev_config_restore(struct rte_eth_dev *dev,
 	return 0;
 }
 
+static int
+eth_dev_config_restore(struct rte_eth_dev *dev,
+		struct rte_eth_dev_info *dev_info, uint16_t port_id)
+{
+	int ret;
+
+	if (!(*dev_info->dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR))
+		eth_dev_mac_restore(dev, dev_info);
+
+	ret = eth_dev_promiscuous_restore(dev, port_id);
+	if (ret != 0)
+		return ret;
+
+	ret = eth_dev_allmulticast_restore(dev, port_id);
+	if (ret != 0)
+		return ret;
+
+	return 0;
+}
+
 int
 rte_eth_dev_start(uint16_t port_id)
 {
-- 
2.39.5

[RFC 2/4] ethdev: omit promiscuous config restore if not required

[Dariusz Sosnowski]

This patch adds a new device flag - RTE_ETH_DEV_PROMISC_FORCE_RESTORE.
If device driver sets this flag, then it requires that ethdev library
forcefully reapplies promiscuous mode configuration,
after the port is started.
As a result, unnecessary work can be removed from rte_eth_dev_start()
for drivers which apply all available configuration in dev_start()
(such drivers do not set the flag).

If RFC is approved, then the next version of this patch
should set the new flag for all drivers to maintain the same behavior,
until drivers adjust and it can be safely cleared.

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
---
 lib/ethdev/rte_ethdev.c | 8 +++++---
 lib/ethdev/rte_ethdev.h | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 362a1883f0..ff08abd566 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1726,9 +1726,11 @@ eth_dev_config_restore(struct rte_eth_dev *dev,
 	if (!(*dev_info->dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR))
 		eth_dev_mac_restore(dev, dev_info);
 
-	ret = eth_dev_promiscuous_restore(dev, port_id);
-	if (ret != 0)
-		return ret;
+	if (*dev_info->dev_flags & RTE_ETH_DEV_PROMISC_FORCE_RESTORE) {
+		ret = eth_dev_promiscuous_restore(dev, port_id);
+		if (ret != 0)
+			return ret;
+	}
 
 	ret = eth_dev_allmulticast_restore(dev, port_id);
 	if (ret != 0)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7..0fc23fb924 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2120,6 +2120,12 @@ struct rte_eth_dev_owner {
  * PMDs filling the queue xstats themselves should not set this flag
  */
 #define RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS RTE_BIT32(6)
+/**
+ * If this flag is set, then device driver requires that
+ * ethdev library forcefully reapplies promiscuous mode configuration,
+ * after driver's dev_start() callback is called.
+ */
+#define RTE_ETH_DEV_PROMISC_FORCE_RESTORE RTE_BIT32(7)
 /**@}*/
 
 /**
-- 
2.39.5

[RFC 3/4] ethdev: omit all multicast config restore if not required

[Dariusz Sosnowski]

This patch adds a new device flag - RTE_ETH_DEV_ALLMULTI_FORCE_RESTORE.
If device driver sets this flag, then it requires that ethdev library
forcefully reapplies allmulticast configration,
after the port is started.
As a result, unnecessary work can be removed from rte_eth_dev_start()
for drivers which apply all available configuration in dev_start()
(such drivers do not set the flag).

If RFC is approved, then the next version of this patch
should set the new flag for all drivers to maintain the same behavior,
until drivers adjust and it can be safely cleared.

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
---
 lib/ethdev/rte_ethdev.c | 8 +++++---
 lib/ethdev/rte_ethdev.h | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index ff08abd566..a08922a78a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1732,9 +1732,11 @@ eth_dev_config_restore(struct rte_eth_dev *dev,
 			return ret;
 	}
 
-	ret = eth_dev_allmulticast_restore(dev, port_id);
-	if (ret != 0)
-		return ret;
+	if (*dev_info->dev_flags & RTE_ETH_DEV_ALLMULTI_FORCE_RESTORE) {
+		ret = eth_dev_allmulticast_restore(dev, port_id);
+		if (ret != 0)
+			return ret;
+	}
 
 	return 0;
 }
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 0fc23fb924..73405dd17d 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2126,6 +2126,12 @@ struct rte_eth_dev_owner {
  * after driver's dev_start() callback is called.
  */
 #define RTE_ETH_DEV_PROMISC_FORCE_RESTORE RTE_BIT32(7)
+/**
+ * If this flag is set, then device driver requires that
+ * ethdev library forcefully reapplies allmulticast configuration,
+ * after driver's dev_start() callback is called.
+ */
+#define RTE_ETH_DEV_ALLMULTI_FORCE_RESTORE RTE_BIT32(8)
 /**@}*/
 
 /**
-- 
2.39.5

[RFC 4/4] ethdev: omit MAC address restore if not required

[Dariusz Sosnowski]

This patch adds a new device flag - RTE_ETH_DEV_MAC_ADDR_FORCE_RESTORE.
If device driver sets this flag, then it requires that ethdev library
forcefully reapplies configured MAC addresses,
after the port is started.
As a result, unnecessary work can be removed from rte_eth_dev_start()
for drivers which apply all available configuration in dev_start()
(such drivers do not the set the flag).

If RFC is approved, then the next version of this patch
should set the new flag for all drivers to maintain the same behavior,
until drivers adjust and it can be safely cleared.

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
---
 lib/ethdev/rte_ethdev.c | 3 ++-
 lib/ethdev/rte_ethdev.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a08922a78a..e4bb40cad8 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1723,7 +1723,8 @@ eth_dev_config_restore(struct rte_eth_dev *dev,
 {
 	int ret;
 
-	if (!(*dev_info->dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR))
+	if ((*dev_info->dev_flags & RTE_ETH_DEV_MAC_ADDR_FORCE_RESTORE) &&
+	    !(*dev_info->dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR))
 		eth_dev_mac_restore(dev, dev_info);
 
 	if (*dev_info->dev_flags & RTE_ETH_DEV_PROMISC_FORCE_RESTORE) {
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 73405dd17d..deab07fbc0 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2132,6 +2132,12 @@ struct rte_eth_dev_owner {
  * after driver's dev_start() callback is called.
  */
 #define RTE_ETH_DEV_ALLMULTI_FORCE_RESTORE RTE_BIT32(8)
+/**
+ * If this flag is set, then device driver requires that
+ * ethdev library forcefully reapplies active MAC addresses,
+ * after driver's dev_start() callback is called.
+ */
+#define RTE_ETH_DEV_MAC_ADDR_FORCE_RESTORE RTE_BIT32(9)
 /**@}*/
 
 /**
-- 
2.39.5