Following are the failure steps:
1. The physical device is removed due to change in one of PF parameters
(e.g. MTU) 2. The interrupt thread flags the device 3. Within 2 seconds
Interrupt thread initializes the actual device removal, then every 2
seconds it tries to re-sync (plug in) the device. The trials fail as
long as VF parameter mismatches the PF parameter.
4. A control thread initiates a control operation on failsafe which
initiates this operation on the device.
5. A race condition occurs between the control thread and interrupt
thread when accessing the device data structures.
This patch mitigates the race condition in step 5.
Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: sta...@dpdk.org
Signed-off-by: Matan Azrad <ma...@mellanox.com>
---
drivers/net/failsafe/failsafe.c | 2 +-
drivers/net/failsafe/failsafe_eal.c | 2 +-
drivers/net/failsafe/failsafe_ether.c | 2 +-
drivers/net/failsafe/failsafe_ops.c | 26 +++++++++++++++++--------
drivers/net/failsafe/failsafe_private.h | 34 ++++++++++++++++++++++++++-------
5 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index 7b2cdbb..6cdefd0 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -187,7 +187,7 @@
* If MAC address was provided as a parameter,
* apply to all probed slaves.
*/
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {
ret = rte_eth_dev_default_mac_addr_set(PORT_ID(sdev),
mac);
if (ret) {
diff --git a/drivers/net/failsafe/failsafe_eal.c
b/drivers/net/failsafe/failsafe_eal.c
index c3d6731..b3b9c32 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -126,7 +126,7 @@
int sdev_ret;
int ret = 0;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {
sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
sdev->dev->name);
if (sdev_ret) {
diff --git a/drivers/net/failsafe/failsafe_ether.c
b/drivers/net/failsafe/failsafe_ether.c
index ca42376..f2a52c9 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -325,7 +325,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)
if (sdev->remove && fs_rxtx_clean(sdev)) {
fs_dev_stats_save(sdev);
fs_dev_remove(sdev);
diff --git a/drivers/net/failsafe/failsafe_ops.c
b/drivers/net/failsafe/failsafe_ops.c
index a7c2dba..3d2cb32 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -181,6 +181,9 @@
FOREACH_SUBDEV(sdev, i, dev) {
if (sdev->state != DEV_ACTIVE)
continue;
+ if (sdev->remove == 1 && PRIV(dev)->state < DEV_STARTED)
+ /* Application shouldn't start removed sub-devices. */
+ continue;
DEBUG("Starting sub_device %d", i);
ret = rte_eth_dev_start(PORT_ID(sdev));
if (ret) {
@@ -265,10 +268,17 @@
uint8_t i;
failsafe_hotplug_alarm_cancel(dev);
- if (PRIV(dev)->state == DEV_STARTED)
+ if (PRIV(dev)->state == DEV_STARTED) {
+ /*
+ * Clean remove flags to allow stop for all sub-devices because
+ * there is not hot-plug alarm to stop the removed sub-devices.
+ */
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_STARTED)
+ sdev->remove = 0;
dev->dev_ops->dev_stop(dev);
+ }
PRIV(dev)->state = DEV_ACTIVE - 1;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Closing sub_device %d", i);
rte_eth_dev_close(PORT_ID(sdev));
sdev->state = DEV_ACTIVE - 1;
@@ -309,7 +319,7 @@
if (rxq->event_fd > 0)
close(rxq->event_fd);
dev = rxq->priv->dev;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)
SUBOPS(sdev, rx_queue_release)
(ETH(sdev)->data->rx_queues[rxq->qid]);
dev->data->rx_queues[rxq->qid] = NULL;
@@ -376,7 +386,7 @@
return ret;
rxq->event_fd = intr_handle.efds[0];
dev->data->rx_queues[rx_queue_id] = rxq;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {
ret = rte_eth_rx_queue_setup(PORT_ID(sdev),
rx_queue_id,
nb_rx_desc, socket_id,
@@ -493,7 +503,7 @@
return;
txq = queue;
dev = txq->priv->dev;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)
SUBOPS(sdev, tx_queue_release)
(ETH(sdev)->data->tx_queues[txq->qid]);
dev->data->tx_queues[txq->qid] = NULL;
@@ -548,7 +558,7 @@
txq->info.nb_desc = nb_tx_desc;
txq->priv = PRIV(dev);
dev->data->tx_queues[tx_queue_id] = txq;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {
ret = rte_eth_tx_queue_setup(PORT_ID(sdev),
tx_queue_id,
nb_tx_desc, socket_id,
@@ -663,7 +673,7 @@
int ret;
rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {
struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
uint64_t *timestamp = &sdev->stats_snapshot.timestamp;
@@ -746,7 +756,7 @@
rx_offload_capa = default_infos.rx_offload_capa;
rxq_offload_capa = default_infos.rx_queue_offload_capa;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {
rte_eth_dev_info_get(PORT_ID(sdev),
&PRIV(dev)->infos);
rx_offload_capa &= PRIV(dev)->infos.rx_offload_capa;
diff --git a/drivers/net/failsafe/failsafe_private.h
b/drivers/net/failsafe/failsafe_private.h
index f3be152..7ddd63a 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -244,16 +244,31 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
((sdev)->sid)
/**
- * Stateful iterator construct over fail-safe sub-devices:
+ * Stateful iterator construct over fail-safe sub-devices,
+ * including the removed sub-devices:
+ * s: (struct sub_device *), iterator
+ * i: (uint8_t), increment
+ * dev: (struct rte_eth_dev *), fail-safe ethdev
+ * state: (enum dev_state), minimum acceptable device state
+ */
+
+#define FOREACH_SUBDEV_STATE_UNSAFE(s, i, dev, state) \
+ for (s = fs_find_next((dev), 0, state, 0, &i); \
+ s != NULL; \
+ s = fs_find_next((dev), i + 1, state, 0, &i))
+
+/**
+ * Stateful iterator construct over fail-safe sub-devices,
+ * except the removed sub-devices:
* s: (struct sub_device *), iterator
* i: (uint8_t), increment
* dev: (struct rte_eth_dev *), fail-safe ethdev
* state: (enum dev_state), minimum acceptable device state
*/
#define FOREACH_SUBDEV_STATE(s, i, dev, state) \
- for (s = fs_find_next((dev), 0, state, &i); \
+ for (s = fs_find_next((dev), 0, state, 1, &i); \
s != NULL; \
- s = fs_find_next((dev), i + 1, state, &i))
+ s = fs_find_next((dev), i + 1, state, 1, &i))
/**
* Iterator construct over fail-safe sub-devices:
@@ -262,7 +277,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
* dev: (struct rte_eth_dev *), fail-safe ethdev
*/
#define FOREACH_SUBDEV(s, i, dev) \
- FOREACH_SUBDEV_STATE(s, i, dev, DEV_UNDEFINED)
+ FOREACH_SUBDEV_STATE_UNSAFE(s, i, dev, DEV_UNDEFINED)
/* dev: (struct rte_eth_dev *) fail-safe device */
#define PREFERRED_SUBDEV(dev) \
@@ -328,6 +343,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
fs_find_next(struct rte_eth_dev *dev,
uint8_t sid,
enum dev_state min_state,
+ uint8_t check_remove,
uint8_t *sid_out)
{
struct sub_device *subs;
@@ -336,8 +352,12 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
subs = PRIV(dev)->subs;
tail = PRIV(dev)->subs_tail;
while (sid < tail) {
- if (subs[sid].state >= min_state)
- break;
+ if (subs[sid].state >= min_state) {
+ if (check_remove == 0)
+ break;
+ if (PRIV(dev)->subs[sid].remove == 0)
+ break;
+ }
sid++;
}
*sid_out = sid;
@@ -376,7 +396,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
uint8_t i;
/* Using acceptable device */
- FOREACH_SUBDEV_STATE(sdev, i, dev, req_state) {
+ FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, req_state) {
if (sdev == banned)
continue;
DEBUG("Switching tx_dev to sub_device %d",
--
1.8.3.1
