From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mellanox.co.il (mail-il-dmz.mellanox.com [193.47.165.129]) by dpdk.org (Postfix) with ESMTP id 575491AEEE for ; Sat, 23 Sep 2017 23:58:09 +0200 (CEST) Received: from Internal Mail-Server by MTLPINE1 (envelope-from ophirmu@mellanox.com) with ESMTPS (AES256-SHA encrypted); 24 Sep 2017 00:58:07 +0300 Received: from pegasus05.mtr.labs.mlnx (pegasus05.mtr.labs.mlnx [10.210.16.100]) by labmailer.mlnx (8.13.8/8.13.8) with ESMTP id v8NLw7fh032641; Sun, 24 Sep 2017 00:58:07 +0300 Received: from pegasus05.mtr.labs.mlnx (localhost [127.0.0.1]) by pegasus05.mtr.labs.mlnx (8.14.7/8.14.7) with ESMTP id v8NLw7SV002768; Sat, 23 Sep 2017 21:58:07 GMT Received: (from root@localhost) by pegasus05.mtr.labs.mlnx (8.14.7/8.14.7/Submit) id v8NLw6L3002765; Sat, 23 Sep 2017 21:58:06 GMT From: Ophir Munk To: Gaetan Rivet Cc: Adrien Mazarguil , dev@dpdk.org, Thomas Monjalon , Olga Shern , Ophir Munk , stable@dpdk.org Date: Sat, 23 Sep 2017 21:57:57 +0000 Message-Id: <1506203877-2090-1-git-send-email-ophirmu@mellanox.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <20170911083117.GM21444@bidouze.vm.6wind.com> References: <20170911083117.GM21444@bidouze.vm.6wind.com> Subject: [dpdk-dev] [PATCH] net/failsafe: fix calling device during RMV events X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Sep 2017 21:58:09 -0000 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 prevents the race condition in step 5. Before this commit if a device was removed and then a control thread operation was initiated on failsafe - in some cases failsafe called the sub device operation instead of avoiding it. Such cases could lead to operations failures. 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 --- This is V2 patch is in reply to <20170911083117.GM21444@bidouze.vm.6wind.com> drivers/net/failsafe/failsafe_ether.c | 1 + drivers/net/failsafe/failsafe_ops.c | 31 +++++++++++++++---------------- drivers/net/failsafe/failsafe_private.h | 26 +++++++++++++++++++++----- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c index a3a8cce..1def110 100644 --- a/drivers/net/failsafe/failsafe_ether.c +++ b/drivers/net/failsafe/failsafe_ether.c @@ -378,6 +378,7 @@ failsafe_eth_dev_state_sync(struct rte_eth_dev *dev) i); goto err_remove; } + sdev->remove = 0; } } /* diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c index ff9ad15..721a48a 100644 --- a/drivers/net/failsafe/failsafe_ops.c +++ b/drivers/net/failsafe/failsafe_ops.c @@ -232,7 +232,6 @@ fs_dev_configure(struct rte_eth_dev *dev) 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, @@ -310,7 +309,7 @@ fs_dev_set_link_up(struct rte_eth_dev *dev) uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { 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) { @@ -329,7 +328,7 @@ fs_dev_set_link_down(struct rte_eth_dev *dev) uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { 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) { @@ -517,7 +516,7 @@ fs_promiscuous_enable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_promiscuous_enable(PORT_ID(sdev)); } @@ -527,7 +526,7 @@ fs_promiscuous_disable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_promiscuous_disable(PORT_ID(sdev)); } @@ -537,7 +536,7 @@ fs_allmulticast_enable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_allmulticast_enable(PORT_ID(sdev)); } @@ -547,7 +546,7 @@ fs_allmulticast_disable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_allmulticast_disable(PORT_ID(sdev)); } @@ -559,7 +558,7 @@ fs_link_update(struct rte_eth_dev *dev, uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { DEBUG("Calling link_update on sub_device %d", i); ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete); if (ret && ret != -1) { @@ -597,7 +596,7 @@ fs_stats_reset(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_stats_reset(PORT_ID(sdev)); } @@ -692,7 +691,7 @@ fs_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { DEBUG("Calling rte_eth_dev_set_mtu on sub_device %d", i); ret = rte_eth_dev_set_mtu(PORT_ID(sdev), mtu); if (ret) { @@ -711,7 +710,7 @@ fs_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { 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) { @@ -745,7 +744,7 @@ fs_flow_ctrl_set(struct rte_eth_dev *dev, uint8_t i; int ret; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { 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) { @@ -766,7 +765,7 @@ fs_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index) /* 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_ACTIVE_SAFE(sdev, i, dev) rte_eth_dev_mac_addr_remove(PORT_ID(sdev), &dev->data->mac_addrs[index]); PRIV(dev)->mac_addr_pool[index] = 0; @@ -783,7 +782,7 @@ fs_mac_addr_add(struct rte_eth_dev *dev, uint8_t i; RTE_ASSERT(index < FAILSAFE_MAX_ETHADDR); - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { ret = rte_eth_dev_mac_addr_add(PORT_ID(sdev), mac_addr, vmdq); if (ret) { ERROR("Operation rte_eth_dev_mac_addr_add failed for sub_device %" @@ -805,7 +804,7 @@ fs_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) struct sub_device *sdev; uint8_t i; - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) rte_eth_dev_default_mac_addr_set(PORT_ID(sdev), mac_addr); } @@ -824,7 +823,7 @@ fs_filter_ctrl(struct rte_eth_dev *dev, *(const void **)arg = &fs_flow_ops; return 0; } - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { + FOREACH_SUBDEV_ACTIVE_SAFE(sdev, i, dev) { 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) { diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h index 0361cf4..fda1606 100644 --- a/drivers/net/failsafe/failsafe_private.h +++ b/drivers/net/failsafe/failsafe_private.h @@ -221,9 +221,21 @@ extern int mac_from_arg; * state: (enum dev_state), minimum acceptable device state */ #define FOREACH_SUBDEV_STATE(s, i, dev, state) \ - for (i = fs_find_next((dev), 0, state); \ + for (i = fs_find_next((dev), 0, state, 0); \ i < PRIV(dev)->subs_tail && (s = &PRIV(dev)->subs[i]); \ - i = fs_find_next((dev), i + 1, state)) + i = fs_find_next((dev), i + 1, state, 0)) + +/** + * Stateful iterator construct over fail-safe sub-devices + * in ACTIVE state and not removed due to RMV event + * s: (struct sub_device *), iterator + * i: (uint8_t), increment + * dev: (struct rte_eth_dev *), fail-safe ethdev + */ +#define FOREACH_SUBDEV_ACTIVE_SAFE(s, i, dev) \ + for (i = fs_find_next((dev), 0, DEV_ACTIVE, 1); \ + i < PRIV(dev)->subs_tail && (s = &PRIV(dev)->subs[i]); \ + i = fs_find_next((dev), i + 1, DEV_ACTIVE, 1)) /** * Iterator construct over fail-safe sub-devices: @@ -296,11 +308,15 @@ extern int mac_from_arg; static inline uint8_t fs_find_next(struct rte_eth_dev *dev, uint8_t sid, - enum dev_state min_state) + enum dev_state min_state, int check_remove) { while (sid < PRIV(dev)->subs_tail) { - if (PRIV(dev)->subs[sid].state >= min_state) - break; + if (PRIV(dev)->subs[sid].state >= min_state) { + if (check_remove == 0) + break; + if (PRIV(dev)->subs[sid].remove == 0) + break; + } sid++; } if (sid >= PRIV(dev)->subs_tail) -- 2.7.4