From 655fcd68c7d282dcd5a414696c3d93d6ccac75da Mon Sep 17 00:00:00 2001 From: Matan Azrad Date: Mon, 12 Feb 2018 20:51:42 +0000 Subject: [PATCH] net/failsafe: fix hotplug races Fail-safe uses a periodic alarm mechanism, running from the host thread, to manage the hot-plug events of its sub-devices. This management requires a lot of sub-devices PMDs operations (stop, close, start, configure, etc.). While the hot-plug alarm runs in the host thread, the application may call fail-safe operations, which directly trigger the sub-devices PMDs operations as well. This call may occur from any thread decided by the application (probably the master thread). Thus, more than one operation can be executed to a sub-device at the same time. This can initiate a lot of races in the sub-PMDs. Moreover, some control operations update the fail-safe internal databases, which can be used by the alarm mechanism at the same time. This can also initiate races and crashes. Fail-safe is the owner of its sub-devices and must synchronize their use according to the ETHDEV ownership rules. Synchronize hot-plug management by a new lock mechanism uses a mutex to atomically defend each critical section in the fail-safe hot-plug mechanism and control operations to prevent any races between them. Fixes: a46f8d5 ("net/failsafe: add fail-safe PMD") Cc: stable@dpdk.org Signed-off-by: Matan Azrad Acked-by: Gaetan Rivet --- drivers/net/failsafe/Makefile | 1 + drivers/net/failsafe/failsafe.c | 35 ++++++ drivers/net/failsafe/failsafe_ether.c | 5 + drivers/net/failsafe/failsafe_flow.c | 20 +++- drivers/net/failsafe/failsafe_ops.c | 148 ++++++++++++++++++++---- drivers/net/failsafe/failsafe_private.h | 56 +++++++++ 6 files changed, 239 insertions(+), 26 deletions(-) diff --git a/drivers/net/failsafe/Makefile b/drivers/net/failsafe/Makefile index d1ae899290..bd2f019863 100644 --- a/drivers/net/failsafe/Makefile +++ b/drivers/net/failsafe/Makefile @@ -68,5 +68,6 @@ CFLAGS += -pedantic LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs LDLIBS += -lrte_bus_vdev +LDLIBS += -lpthread include $(RTE_SDK)/mk/rte.lib.mk diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c index 7b2cdbba2c..c499bfb984 100644 --- a/drivers/net/failsafe/failsafe.c +++ b/drivers/net/failsafe/failsafe.c @@ -113,16 +113,45 @@ fs_hotplug_alarm(void *arg) break; /* if we have non-probed device */ if (i != PRIV(dev)->subs_tail) { + if (fs_lock(dev, 1) != 0) + goto reinstall; ret = failsafe_eth_dev_state_sync(dev); + fs_unlock(dev, 1); if (ret) ERROR("Unable to synchronize sub_device state"); } failsafe_dev_remove(dev); +reinstall: ret = failsafe_hotplug_alarm_install(dev); if (ret) ERROR("Unable to set up next alarm"); } +static int +fs_mutex_init(struct fs_priv *priv) +{ + int ret; + pthread_mutexattr_t attr; + + ret = pthread_mutexattr_init(&attr); + if (ret) { + ERROR("Cannot initiate mutex attributes - %s", strerror(ret)); + return ret; + } + /* Allow mutex relocks for the thread holding the mutex. */ + ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + if (ret) { + ERROR("Cannot set mutex type - %s", strerror(ret)); + return ret; + } + ret = pthread_mutex_init(&priv->hotplug_mutex, &attr); + if (ret) { + ERROR("Cannot initiate mutex - %s", strerror(ret)); + return ret; + } + return 0; +} + static int fs_eth_dev_create(struct rte_vdev_device *vdev) { @@ -174,6 +203,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) snprintf(priv->my_owner.name, sizeof(priv->my_owner.name), FAILSAFE_OWNER_NAME); ret = failsafe_eal_init(dev); + if (ret) + goto free_args; + ret = fs_mutex_init(priv); if (ret) goto free_args; ret = failsafe_hotplug_alarm_install(dev); @@ -250,6 +282,9 @@ fs_rte_eth_free(const char *name) ERROR("Error while uninitializing sub-EAL"); failsafe_args_free(dev); fs_sub_device_free(dev); + ret = pthread_mutex_destroy(&PRIV(dev)->hotplug_mutex); + if (ret) + ERROR("Error while destroying hotplug mutex"); rte_free(PRIV(dev)); rte_eth_dev_release_port(dev); return ret; diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c index d820fafd87..2c0bf9366b 100644 --- a/drivers/net/failsafe/failsafe_ether.c +++ b/drivers/net/failsafe/failsafe_ether.c @@ -328,8 +328,11 @@ failsafe_dev_remove(struct rte_eth_dev *dev) FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) if (sdev->remove && fs_rxtx_clean(sdev)) { + if (fs_lock(dev, 1) != 0) + return; fs_dev_stats_save(sdev); fs_dev_remove(sdev); + fs_unlock(dev, 1); } } @@ -429,6 +432,7 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused, { struct sub_device *sdev = cb_arg; + fs_lock(sdev->fs_dev, 0); /* Switch as soon as possible tx_dev. */ fs_switch_dev(sdev->fs_dev, sdev); /* Use safe bursts in any case. */ @@ -438,6 +442,7 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused, * the callback at the source of the current thread context. */ sdev->remove = 1; + fs_unlock(sdev->fs_dev, 0); return 0; } diff --git a/drivers/net/failsafe/failsafe_flow.c b/drivers/net/failsafe/failsafe_flow.c index 4d18e8ee73..ec8c909b20 100644 --- a/drivers/net/failsafe/failsafe_flow.c +++ b/drivers/net/failsafe/failsafe_flow.c @@ -55,6 +55,7 @@ fs_flow_validate(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { DEBUG("Calling rte_flow_validate on sub_device %d", i); ret = rte_flow_validate(PORT_ID(sdev), @@ -62,9 +63,11 @@ fs_flow_validate(struct rte_eth_dev *dev, if ((ret = fs_err(sdev, ret))) { ERROR("Operation rte_flow_validate failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -79,6 +82,7 @@ fs_flow_create(struct rte_eth_dev *dev, struct rte_flow *flow; uint8_t i; + fs_lock(dev, 0); flow = fs_flow_allocate(attr, patterns, actions); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { flow->flows[i] = rte_flow_create(PORT_ID(sdev), @@ -90,6 +94,7 @@ fs_flow_create(struct rte_eth_dev *dev, } } TAILQ_INSERT_TAIL(&PRIV(dev)->flow_list, flow, next); + fs_unlock(dev, 0); return flow; err: FOREACH_SUBDEV(sdev, i, dev) { @@ -98,6 +103,7 @@ err: flow->flows[i], error); } fs_flow_release(&flow); + fs_unlock(dev, 0); return NULL; } @@ -115,6 +121,7 @@ fs_flow_destroy(struct rte_eth_dev *dev, return -EINVAL; } ret = 0; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { int local_ret; @@ -131,6 +138,7 @@ fs_flow_destroy(struct rte_eth_dev *dev, } TAILQ_REMOVE(&PRIV(dev)->flow_list, flow, next); fs_flow_release(&flow); + fs_unlock(dev, 0); return ret; } @@ -144,12 +152,14 @@ fs_flow_flush(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_flow_flush failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } @@ -157,6 +167,7 @@ fs_flow_flush(struct rte_eth_dev *dev, TAILQ_REMOVE(&PRIV(dev)->flow_list, flow, next); fs_flow_release(&flow); } + fs_unlock(dev, 0); return 0; } @@ -169,15 +180,19 @@ fs_flow_query(struct rte_eth_dev *dev, { struct sub_device *sdev; + fs_lock(dev, 0); sdev = TX_SUBDEV(dev); if (sdev != NULL) { int ret = rte_flow_query(PORT_ID(sdev), flow->flows[SUB_ID(sdev)], type, arg, error); - if ((ret = fs_err(sdev, ret))) + if ((ret = fs_err(sdev, ret))) { + fs_unlock(dev, 0); return ret; + } } + fs_unlock(dev, 0); WARN("No active sub_device to query about its flow"); return -1; } @@ -191,6 +206,7 @@ fs_flow_isolate(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV(sdev, i, dev) { if (sdev->state < DEV_PROBED) continue; @@ -202,11 +218,13 @@ fs_flow_isolate(struct rte_eth_dev *dev, if ((ret = fs_err(sdev, ret))) { ERROR("Operation rte_flow_isolate failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } sdev->flow_isolated = set; } PRIV(dev)->flow_isolated = set; + fs_unlock(dev, 0); return 0; } diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c index f0e48c1880..fe64c68485 100644 --- a/drivers/net/failsafe/failsafe_ops.c +++ b/drivers/net/failsafe/failsafe_ops.c @@ -94,6 +94,7 @@ fs_dev_configure(struct rte_eth_dev *dev) uint8_t i; int ret; + fs_lock(dev, 0); supp_tx_offloads = PRIV(dev)->infos.tx_offload_capa; tx_offloads = dev->data->dev_conf.txmode.offloads; if ((tx_offloads & supp_tx_offloads) != tx_offloads) { @@ -101,6 +102,7 @@ fs_dev_configure(struct rte_eth_dev *dev) ERROR("Some Tx offloads are not supported, " "requested 0x%" PRIx64 " supported 0x%" PRIx64, tx_offloads, supp_tx_offloads); + fs_unlock(dev, 0); return -rte_errno; } FOREACH_SUBDEV(sdev, i, dev) { @@ -139,6 +141,7 @@ fs_dev_configure(struct rte_eth_dev *dev) if (!fs_err(sdev, ret)) continue; ERROR("Could not configure sub_device %d", i); + fs_unlock(dev, 0); return ret; } if (rmv_interrupt) { @@ -165,6 +168,7 @@ fs_dev_configure(struct rte_eth_dev *dev) } if (PRIV(dev)->state < DEV_ACTIVE) PRIV(dev)->state = DEV_ACTIVE; + fs_unlock(dev, 0); return 0; } @@ -175,9 +179,12 @@ fs_dev_start(struct rte_eth_dev *dev) uint8_t i; int ret; + fs_lock(dev, 0); ret = failsafe_rx_intr_install(dev); - if (ret) + if (ret) { + fs_unlock(dev, 0); return ret; + } FOREACH_SUBDEV(sdev, i, dev) { if (sdev->state != DEV_ACTIVE) continue; @@ -186,6 +193,7 @@ fs_dev_start(struct rte_eth_dev *dev) if (ret) { if (!fs_err(sdev, ret)) continue; + fs_unlock(dev, 0); return ret; } ret = failsafe_rx_intr_install_subdevice(sdev); @@ -193,6 +201,7 @@ fs_dev_start(struct rte_eth_dev *dev) if (!fs_err(sdev, ret)) continue; rte_eth_dev_stop(PORT_ID(sdev)); + fs_unlock(dev, 0); return ret; } sdev->state = DEV_STARTED; @@ -200,6 +209,7 @@ fs_dev_start(struct rte_eth_dev *dev) if (PRIV(dev)->state < DEV_STARTED) PRIV(dev)->state = DEV_STARTED; fs_switch_dev(dev, NULL); + fs_unlock(dev, 0); return 0; } @@ -209,6 +219,7 @@ fs_dev_stop(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); PRIV(dev)->state = DEV_STARTED - 1; FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_STARTED) { rte_eth_dev_stop(PORT_ID(sdev)); @@ -216,6 +227,7 @@ fs_dev_stop(struct rte_eth_dev *dev) sdev->state = DEV_STARTED - 1; } failsafe_rx_intr_uninstall(dev); + fs_unlock(dev, 0); } static int @@ -225,15 +237,18 @@ fs_dev_set_link_up(struct rte_eth_dev *dev) uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_set_link_up failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -244,15 +259,18 @@ fs_dev_set_link_down(struct rte_eth_dev *dev) uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_set_link_down failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -263,6 +281,7 @@ fs_dev_close(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); failsafe_hotplug_alarm_cancel(dev); if (PRIV(dev)->state == DEV_STARTED) dev->dev_ops->dev_stop(dev); @@ -273,6 +292,7 @@ fs_dev_close(struct rte_eth_dev *dev) sdev->state = DEV_ACTIVE - 1; } fs_dev_free_queues(dev); + fs_unlock(dev, 0); } static bool @@ -305,14 +325,16 @@ fs_rx_queue_release(void *queue) if (queue == NULL) return; rxq = queue; + dev = rxq->priv->dev; + fs_lock(dev, 0); if (rxq->event_fd > 0) close(rxq->event_fd); - dev = rxq->priv->dev; FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) SUBOPS(sdev, rx_queue_release) (ETH(sdev)->data->rx_queues[rxq->qid]); dev->data->rx_queues[rxq->qid] = NULL; rte_free(rxq); + fs_unlock(dev, 0); } static int @@ -338,6 +360,7 @@ fs_rx_queue_setup(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); rxq = dev->data->rx_queues[rx_queue_id]; if (rxq != NULL) { fs_rx_queue_release(rxq); @@ -353,14 +376,17 @@ fs_rx_queue_setup(struct rte_eth_dev *dev, dev->data->dev_conf.rxmode.offloads, PRIV(dev)->infos.rx_offload_capa | PRIV(dev)->infos.rx_queue_offload_capa); + fs_unlock(dev, 0); return -rte_errno; } rxq = rte_zmalloc(NULL, sizeof(*rxq) + sizeof(rte_atomic64_t) * PRIV(dev)->subs_tail, RTE_CACHE_LINE_SIZE); - if (rxq == NULL) + if (rxq == NULL) { + fs_unlock(dev, 0); return -ENOMEM; + } FOREACH_SUBDEV(sdev, i, dev) rte_atomic64_init(&rxq->refcnt[i]); rxq->qid = rx_queue_id; @@ -371,8 +397,10 @@ fs_rx_queue_setup(struct rte_eth_dev *dev, rxq->priv = PRIV(dev); rxq->sdev = PRIV(dev)->subs; ret = rte_intr_efd_enable(&intr_handle, 1); - if (ret < 0) + if (ret < 0) { + fs_unlock(dev, 0); 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) { @@ -385,9 +413,11 @@ fs_rx_queue_setup(struct rte_eth_dev *dev, goto free_rxq; } } + fs_unlock(dev, 0); return 0; free_rxq: fs_rx_queue_release(rxq); + fs_unlock(dev, 0); return ret; } @@ -400,20 +430,21 @@ fs_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx) int ret; int rc = 0; + fs_lock(dev, 0); if (idx >= dev->data->nb_rx_queues) { - rte_errno = EINVAL; - return -rte_errno; + rc = -EINVAL; + goto unlock; } rxq = dev->data->rx_queues[idx]; if (rxq == NULL || rxq->event_fd <= 0) { - rte_errno = EINVAL; - return -rte_errno; + rc = -EINVAL; + goto unlock; } /* Fail if proxy service is nor running. */ if (PRIV(dev)->rxp.sstate != SS_RUNNING) { ERROR("failsafe interrupt services are not running"); - rte_errno = EAGAIN; - return -rte_errno; + rc = -EAGAIN; + goto unlock; } rxq->enable_events = 1; FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { @@ -422,6 +453,8 @@ fs_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx) if (ret) rc = ret; } +unlock: + fs_unlock(dev, 0); if (rc) rte_errno = -rc; return rc; @@ -437,14 +470,15 @@ fs_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx) int rc = 0; int ret; + fs_lock(dev, 0); if (idx >= dev->data->nb_rx_queues) { - rte_errno = EINVAL; - return -rte_errno; + rc = -EINVAL; + goto unlock; } rxq = dev->data->rx_queues[idx]; if (rxq == NULL || rxq->event_fd <= 0) { - rte_errno = EINVAL; - return -rte_errno; + rc = -EINVAL; + goto unlock; } rxq->enable_events = 0; FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { @@ -456,6 +490,8 @@ fs_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx) /* Clear pending events */ while (read(rxq->event_fd, &u64, sizeof(uint64_t)) > 0) ; +unlock: + fs_unlock(dev, 0); if (rc) rte_errno = -rc; return rc; @@ -492,11 +528,13 @@ fs_tx_queue_release(void *queue) return; txq = queue; dev = txq->priv->dev; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) SUBOPS(sdev, tx_queue_release) (ETH(sdev)->data->tx_queues[txq->qid]); dev->data->tx_queues[txq->qid] = NULL; rte_free(txq); + fs_unlock(dev, 0); } static int @@ -511,6 +549,7 @@ fs_tx_queue_setup(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); txq = dev->data->tx_queues[tx_queue_id]; if (txq != NULL) { fs_tx_queue_release(txq); @@ -531,14 +570,17 @@ fs_tx_queue_setup(struct rte_eth_dev *dev, dev->data->dev_conf.txmode.offloads, PRIV(dev)->infos.tx_offload_capa | PRIV(dev)->infos.tx_queue_offload_capa); + fs_unlock(dev, 0); return -rte_errno; } txq = rte_zmalloc("ethdev TX queue", sizeof(*txq) + sizeof(rte_atomic64_t) * PRIV(dev)->subs_tail, RTE_CACHE_LINE_SIZE); - if (txq == NULL) + if (txq == NULL) { + fs_unlock(dev, 0); return -ENOMEM; + } FOREACH_SUBDEV(sdev, i, dev) rte_atomic64_init(&txq->refcnt[i]); txq->qid = tx_queue_id; @@ -557,9 +599,11 @@ fs_tx_queue_setup(struct rte_eth_dev *dev, goto free_txq; } } + fs_unlock(dev, 0); return 0; free_txq: fs_tx_queue_release(txq); + fs_unlock(dev, 0); return ret; } @@ -586,8 +630,10 @@ fs_promiscuous_enable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) rte_eth_promiscuous_enable(PORT_ID(sdev)); + fs_unlock(dev, 0); } static void @@ -596,8 +642,10 @@ fs_promiscuous_disable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) rte_eth_promiscuous_disable(PORT_ID(sdev)); + fs_unlock(dev, 0); } static void @@ -606,8 +654,10 @@ fs_allmulticast_enable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) rte_eth_allmulticast_enable(PORT_ID(sdev)); + fs_unlock(dev, 0); } static void @@ -616,8 +666,10 @@ fs_allmulticast_disable(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) rte_eth_allmulticast_disable(PORT_ID(sdev)); + fs_unlock(dev, 0); } static int @@ -628,6 +680,7 @@ fs_link_update(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { DEBUG("Calling link_update on sub_device %d", i); ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete); @@ -635,6 +688,7 @@ fs_link_update(struct rte_eth_dev *dev, rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) { ERROR("Link update failed for sub_device %d with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } @@ -646,9 +700,11 @@ fs_link_update(struct rte_eth_dev *dev, l2 = Ð(TX_SUBDEV(dev))->data->dev_link; if (memcmp(l1, l2, sizeof(*l1))) { *l1 = *l2; + fs_unlock(dev, 0); return 0; } } + fs_unlock(dev, 0); return -1; } @@ -661,6 +717,7 @@ fs_stats_get(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats)); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats; @@ -676,12 +733,14 @@ fs_stats_get(struct rte_eth_dev *dev, ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d", i, ret); *timestamp = 0; + fs_unlock(dev, 0); return ret; } *timestamp = rte_rdtsc(); inc: failsafe_stats_increment(stats, snapshot); } + fs_unlock(dev, 0); return 0; } @@ -691,11 +750,13 @@ fs_stats_reset(struct rte_eth_dev *dev) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { rte_eth_stats_reset(PORT_ID(sdev)); memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats)); } memset(&PRIV(dev)->stats_accumulator, 0, sizeof(struct rte_eth_stats)); + fs_unlock(dev, 0); } /** @@ -771,14 +832,20 @@ fs_dev_supported_ptypes_get(struct rte_eth_dev *dev) { struct sub_device *sdev; struct rte_eth_dev *edev; + const uint32_t *ret; + fs_lock(dev, 0); sdev = TX_SUBDEV(dev); - if (sdev == NULL) - return NULL; + if (sdev == NULL) { + ret = NULL; + goto unlock; + } edev = ETH(sdev); /* ENOTSUP: counts as no supported ptypes */ - if (SUBOPS(sdev, dev_supported_ptypes_get) == NULL) - return NULL; + if (SUBOPS(sdev, dev_supported_ptypes_get) == NULL) { + ret = NULL; + goto unlock; + } /* * The API does not permit to do a clean AND of all ptypes, * It is also incomplete by design and we do not really care @@ -786,7 +853,10 @@ fs_dev_supported_ptypes_get(struct rte_eth_dev *dev) * We just return the ptypes of the device of highest * priority, usually the PREFERRED device. */ - return SUBOPS(sdev, dev_supported_ptypes_get)(edev); + ret = SUBOPS(sdev, dev_supported_ptypes_get)(edev); +unlock: + fs_unlock(dev, 0); + return ret; } static int @@ -796,15 +866,18 @@ fs_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_set_mtu failed for sub_device %d with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -815,15 +888,18 @@ fs_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_vlan_filter failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -832,13 +908,22 @@ fs_flow_ctrl_get(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf) { struct sub_device *sdev; + int ret; + fs_lock(dev, 0); sdev = TX_SUBDEV(dev); - if (sdev == NULL) - return 0; - if (SUBOPS(sdev, flow_ctrl_get) == NULL) - return -ENOTSUP; - return SUBOPS(sdev, flow_ctrl_get)(ETH(sdev), fc_conf); + if (sdev == NULL) { + ret = 0; + goto unlock; + } + if (SUBOPS(sdev, flow_ctrl_get) == NULL) { + ret = -ENOTSUP; + goto unlock; + } + ret = SUBOPS(sdev, flow_ctrl_get)(ETH(sdev), fc_conf); +unlock: + fs_unlock(dev, 0); + return ret; } static int @@ -849,15 +934,18 @@ fs_flow_ctrl_set(struct rte_eth_dev *dev, uint8_t i; int ret; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_flow_ctrl_set failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } @@ -867,6 +955,7 @@ fs_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); /* No check: already done within the rte_eth_dev_mac_addr_remove * call for the fail-safe device. */ @@ -874,6 +963,7 @@ fs_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index) rte_eth_dev_mac_addr_remove(PORT_ID(sdev), &dev->data->mac_addrs[index]); PRIV(dev)->mac_addr_pool[index] = 0; + fs_unlock(dev, 0); } static int @@ -887,11 +977,13 @@ fs_mac_addr_add(struct rte_eth_dev *dev, uint8_t i; RTE_ASSERT(index < FAILSAFE_MAX_ETHADDR); + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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 %" PRIu8 " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } @@ -900,6 +992,7 @@ fs_mac_addr_add(struct rte_eth_dev *dev, PRIV(dev)->nb_mac_addr = index; } PRIV(dev)->mac_addr_pool[index] = vmdq; + fs_unlock(dev, 0); return 0; } @@ -909,8 +1002,10 @@ fs_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) struct sub_device *sdev; uint8_t i; + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) rte_eth_dev_default_mac_addr_set(PORT_ID(sdev), mac_addr); + fs_unlock(dev, 0); } static int @@ -928,15 +1023,18 @@ fs_filter_ctrl(struct rte_eth_dev *dev, *(const void **)arg = &fs_flow_ops; return 0; } + fs_lock(dev, 0); FOREACH_SUBDEV_STATE(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))) { ERROR("Operation rte_eth_dev_filter_ctrl failed for sub_device %d" " with error %d", i, ret); + fs_unlock(dev, 0); return ret; } } + fs_unlock(dev, 0); return 0; } diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h index f3be1528fd..5b84db9056 100644 --- a/drivers/net/failsafe/failsafe_private.h +++ b/drivers/net/failsafe/failsafe_private.h @@ -7,6 +7,7 @@ #define _RTE_ETH_FAILSAFE_PRIVATE_H_ #include +#include #include #include @@ -161,6 +162,9 @@ struct fs_priv { * appropriate failsafe Rx queue. */ struct rx_proxy rxp; + pthread_mutex_t hotplug_mutex; + /* Hot-plug mutex is locked by the alarm mechanism. */ + volatile unsigned int alarm_lock:1; unsigned int pending_alarm:1; /* An alarm is pending */ /* flow isolation state */ int flow_isolated:1; @@ -346,6 +350,58 @@ fs_find_next(struct rte_eth_dev *dev, return &subs[sid]; } +/* + * Lock hot-plug mutex. + * is_alarm means that the caller is, for sure, the hot-plug alarm mechanism. + */ +static inline int +fs_lock(struct rte_eth_dev *dev, unsigned int is_alarm) +{ + int ret; + + if (is_alarm) { + ret = pthread_mutex_trylock(&PRIV(dev)->hotplug_mutex); + if (ret) { + DEBUG("Hot-plug mutex lock trying failed(%s), will try" + " again later...", strerror(ret)); + return ret; + } + PRIV(dev)->alarm_lock = 1; + } else { + ret = pthread_mutex_lock(&PRIV(dev)->hotplug_mutex); + if (ret) { + ERROR("Cannot lock mutex(%s)", strerror(ret)); + return ret; + } + } + DEBUG("Hot-plug mutex was locked by thread %lu%s", pthread_self(), + PRIV(dev)->alarm_lock ? " by the hot-plug alarm" : ""); + return ret; +} + +/* + * Unlock hot-plug mutex. + * is_alarm means that the caller is, for sure, the hot-plug alarm mechanism. + */ +static inline void +fs_unlock(struct rte_eth_dev *dev, unsigned int is_alarm) +{ + int ret; + unsigned int prev_alarm_lock = PRIV(dev)->alarm_lock; + + if (is_alarm) { + RTE_ASSERT(PRIV(dev)->alarm_lock == 1); + PRIV(dev)->alarm_lock = 0; + } + ret = pthread_mutex_unlock(&PRIV(dev)->hotplug_mutex); + if (ret) + ERROR("Cannot unlock hot-plug mutex(%s)", strerror(ret)); + else + DEBUG("Hot-plug mutex was unlocked by thread %lu%s", + pthread_self(), + prev_alarm_lock ? " by the hot-plug alarm" : ""); +} + /* * Switch emitting device. * If banned is set, banned must not be considered for -- 2.20.1