From: Matan Azrad <matan@mellanox.com>
To: Gaetan Rivet <gaetan.rivet@6wind.com>
Cc: dev@dpdk.org, Ophir Munk <ophirmu@mellanox.com>, stable@dpdk.org
Subject: [dpdk-dev] [PATCH v4 2/2] net/failsafe: fix calling device during RMV events
Date: Thu, 8 Feb 2018 12:20:27 +0000 [thread overview]
Message-ID: <1518092427-4333-3-git-send-email-matan@mellanox.com> (raw)
In-Reply-To: <1518092427-4333-1-git-send-email-matan@mellanox.com>
From: Ophir Munk <ophirmu@mellanox.com>
This commit prevents control path operations from failing after a sub
device removal.
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 commit mitigates the race condition in step 5.
This commit fixes failsafe criteria to determine when the device is removed
such that it will avoid calling the sub device operations during that time
and will only call them otherwise.
Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org
Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
Signed-off-by: Matan Azrad <matan@mellanox.com>
---
drivers/net/failsafe/failsafe_ether.c | 2 ++
drivers/net/failsafe/failsafe_flow.c | 8 +++---
drivers/net/failsafe/failsafe_ops.c | 50 ++++++++++++++++++++-------------
drivers/net/failsafe/failsafe_private.h | 26 ++++++++++++++---
4 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 4c6e938..ca42376 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -377,6 +377,8 @@
i);
goto err_remove;
}
+ if (PRIV(dev)->state < DEV_STARTED)
+ sdev->remove = 0;
}
}
/*
diff --git a/drivers/net/failsafe/failsafe_flow.c b/drivers/net/failsafe/failsafe_flow.c
index 4d18e8e..d4a69cf 100644
--- a/drivers/net/failsafe/failsafe_flow.c
+++ b/drivers/net/failsafe/failsafe_flow.c
@@ -55,7 +55,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_flow_validate on sub_device %d", i);
ret = rte_flow_validate(PORT_ID(sdev),
attr, patterns, actions, error);
@@ -80,7 +80,7 @@
uint8_t i;
flow = fs_flow_allocate(attr, patterns, actions);
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
flow->flows[i] = rte_flow_create(PORT_ID(sdev),
attr, patterns, actions, error);
if (flow->flows[i] == NULL && fs_err(sdev, -rte_errno)) {
@@ -115,7 +115,7 @@
return -EINVAL;
}
ret = 0;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
int local_ret;
if (flow->flows[i] == NULL)
@@ -144,7 +144,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_flow_flush on sub_device %d", i);
ret = rte_flow_flush(PORT_ID(sdev), error);
if ((ret = fs_err(sdev, ret))) {
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 7a67e16..3312cb2 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -131,7 +131,6 @@
dev->data->dev_conf.intr_conf.lsc = 0;
}
DEBUG("Configuring sub-device %d", i);
- sdev->remove = 0;
ret = rte_eth_dev_configure(PORT_ID(sdev),
dev->data->nb_rx_queues,
dev->data->nb_tx_queues,
@@ -182,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) {
@@ -197,6 +199,7 @@
return ret;
}
sdev->state = DEV_STARTED;
+ sdev->remove = 0;
}
if (PRIV(dev)->state < DEV_STARTED)
PRIV(dev)->state = DEV_STARTED;
@@ -211,7 +214,7 @@
uint8_t i;
PRIV(dev)->state = DEV_STARTED - 1;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_STARTED) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_STARTED) {
rte_eth_dev_stop(PORT_ID(sdev));
failsafe_rx_intr_uninstall_subdevice(sdev);
sdev->state = DEV_STARTED - 1;
@@ -226,7 +229,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_set_link_up on sub_device %d", i);
ret = rte_eth_dev_set_link_up(PORT_ID(sdev));
if ((ret = fs_err(sdev, ret))) {
@@ -245,7 +248,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_set_link_down on sub_device %d", i);
ret = rte_eth_dev_set_link_down(PORT_ID(sdev));
if ((ret = fs_err(sdev, ret))) {
@@ -265,8 +268,15 @@
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(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) {
DEBUG("Closing sub_device %d", i);
@@ -417,7 +427,7 @@
return -rte_errno;
}
rxq->enable_events = 1;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
ret = rte_eth_dev_rx_intr_enable(PORT_ID(sdev), idx);
ret = fs_err(sdev, ret);
if (ret)
@@ -448,7 +458,7 @@
return -rte_errno;
}
rxq->enable_events = 0;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
ret = rte_eth_dev_rx_intr_disable(PORT_ID(sdev), idx);
ret = fs_err(sdev, ret);
if (ret)
@@ -587,7 +597,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_promiscuous_enable(PORT_ID(sdev));
}
@@ -597,7 +607,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_promiscuous_disable(PORT_ID(sdev));
}
@@ -607,7 +617,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_allmulticast_enable(PORT_ID(sdev));
}
@@ -617,7 +627,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_allmulticast_disable(PORT_ID(sdev));
}
@@ -629,7 +639,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling link_update on sub_device %d", i);
ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete);
if (ret && ret != -1 && sdev->remove == 0 &&
@@ -692,7 +702,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
rte_eth_stats_reset(PORT_ID(sdev));
memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
}
@@ -797,7 +807,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_set_mtu on sub_device %d", i);
ret = rte_eth_dev_set_mtu(PORT_ID(sdev), mtu);
if ((ret = fs_err(sdev, ret))) {
@@ -816,7 +826,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_vlan_filter on sub_device %d", i);
ret = rte_eth_dev_vlan_filter(PORT_ID(sdev), vlan_id, on);
if ((ret = fs_err(sdev, ret))) {
@@ -850,7 +860,7 @@
uint8_t i;
int ret;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_flow_ctrl_set on sub_device %d", i);
ret = rte_eth_dev_flow_ctrl_set(PORT_ID(sdev), fc_conf);
if ((ret = fs_err(sdev, ret))) {
@@ -871,7 +881,7 @@
/* No check: already done within the rte_eth_dev_mac_addr_remove
* call for the fail-safe device.
*/
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_dev_mac_addr_remove(PORT_ID(sdev),
&dev->data->mac_addrs[index]);
PRIV(dev)->mac_addr_pool[index] = 0;
@@ -888,7 +898,7 @@
uint8_t i;
RTE_ASSERT(index < FAILSAFE_MAX_ETHADDR);
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
ret = rte_eth_dev_mac_addr_add(PORT_ID(sdev), mac_addr, vmdq);
if ((ret = fs_err(sdev, ret))) {
ERROR("Operation rte_eth_dev_mac_addr_add failed for sub_device %"
@@ -910,7 +920,7 @@
struct sub_device *sdev;
uint8_t i;
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE)
rte_eth_dev_default_mac_addr_set(PORT_ID(sdev), mac_addr);
}
@@ -929,7 +939,7 @@
*(const void **)arg = &fs_flow_ops;
return 0;
}
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ FOREACH_SUBDEV_STATE_SAFE(sdev, i, dev, DEV_ACTIVE) {
DEBUG("Calling rte_eth_dev_filter_ctrl on sub_device %d", i);
ret = rte_eth_dev_filter_ctrl(PORT_ID(sdev), type, op, arg);
if ((ret = fs_err(sdev, ret))) {
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index f3be152..0f3b543 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -250,10 +250,23 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
* 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, 0, &i); \
s != NULL; \
- s = fs_find_next((dev), i + 1, state, &i))
+ s = fs_find_next((dev), i + 1, state, 0, &i))
+
+/**
+ * Stateful iterator construct over fail-safe safe 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_SAFE(s, i, dev, state) \
+ for (s = fs_find_next((dev), 0, state, 1, &i); \
+ s != NULL; \
+ s = fs_find_next((dev), i + 1, state, 1, &i))
/**
* Iterator construct over fail-safe sub-devices:
@@ -328,6 +341,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 +350,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;
--
1.8.3.1
next prev parent reply other threads:[~2018-02-08 12:20 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-09 19:27 [dpdk-dev] [PATCH] " Ophir Munk
2017-09-11 8:31 ` Gaëtan Rivet
2017-09-23 21:57 ` Ophir Munk
2017-10-05 22:42 ` [dpdk-dev] [PATCH v3] " Ophir Munk
2017-10-20 10:35 ` Gaëtan Rivet
2017-10-23 7:17 ` Ophir Munk
2017-10-23 8:36 ` Gaëtan Rivet
2017-11-29 19:17 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2018-01-18 22:22 ` Thomas Monjalon
2018-01-18 23:35 ` Gaëtan Rivet
2018-02-08 12:20 ` [dpdk-dev] [PATCH v4 0/2] failsafe: " Matan Azrad
2018-02-08 12:20 ` [dpdk-dev] [PATCH v4 1/2] net/failsafe: fix hotplug alarm cancel Matan Azrad
2018-02-08 12:20 ` Matan Azrad [this message]
2018-02-08 16:34 ` [dpdk-dev] [PATCH v5 0/3] failsafe: fix calling device during RMV events Matan Azrad
2018-02-08 16:34 ` [dpdk-dev] [PATCH v5 1/3] net/failsafe: fix hotplug alarm cancel Matan Azrad
2018-02-08 16:34 ` [dpdk-dev] [PATCH v5 2/3] net/failsafe: fix removal scope Matan Azrad
2018-02-08 17:19 ` Gaëtan Rivet
2018-02-08 19:03 ` Matan Azrad
2018-02-08 16:34 ` [dpdk-dev] [PATCH v5 3/3] net/failsafe: fix calling device during RMV events Matan Azrad
2018-02-08 18:11 ` Gaëtan Rivet
2018-02-08 19:24 ` Matan Azrad
2018-02-11 17:24 ` [dpdk-dev] [PATCH v6 0/3] failsafe: fix hotplug races Matan Azrad
2018-02-11 17:24 ` [dpdk-dev] [PATCH v6 1/3] net/failsafe: fix hotplug alarm cancel Matan Azrad
2018-02-11 17:24 ` [dpdk-dev] [PATCH v6 2/3] net/failsafe: fix removal scope Matan Azrad
2018-02-11 17:24 ` [dpdk-dev] [PATCH v6 3/3] net/failsafe: fix hotplug races Matan Azrad
2018-02-12 18:33 ` Gaëtan Rivet
2018-02-12 20:35 ` Matan Azrad
2018-02-12 20:51 ` [dpdk-dev] [PATCH v7 0/3] failsafe: " Matan Azrad
2018-02-12 20:51 ` [dpdk-dev] [PATCH v7 1/3] net/failsafe: fix hotplug alarm cancel Matan Azrad
2018-02-12 20:51 ` [dpdk-dev] [PATCH v7 2/3] net/failsafe: fix removal scope Matan Azrad
2018-02-12 20:51 ` [dpdk-dev] [PATCH v7 3/3] net/failsafe: fix hotplug races Matan Azrad
2018-02-13 13:31 ` [dpdk-dev] [PATCH v7 0/3] failsafe: " Gaëtan Rivet
2018-02-13 16:12 ` Thomas Monjalon
2018-02-13 20:58 ` De Lara Guarch, Pablo
2018-02-13 21:13 ` Matan Azrad
2018-02-13 21:21 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1518092427-4333-3-git-send-email-matan@mellanox.com \
--to=matan@mellanox.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=ophirmu@mellanox.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).