From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id 6C4677D97 for ; Fri, 1 Sep 2017 10:08:01 +0200 (CEST) Received: by mail-wr0-f179.google.com with SMTP id y15so4497898wrc.2 for ; Fri, 01 Sep 2017 01:08:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:in-reply-to:references; bh=+rsli49ymfYIDDcZJ/yXC1Y4r08cVD11liWxP5aLvvE=; b=EH+Ik4dwJSZbdpMAzF3D8xocwpKpZxixHOIpi90MUfO9X4jJ4id3d2tBy4Gh++ydLR XMwojHb8H5N1p+YC9eHOwWBmS+JNnjhLMOMDgY9BFUoN6ob96TJP8UeQ/4ZaeuCsF5D2 iSh8CneTCTAI2P49RNwFy0CUWCcwaOyMiHDkW4LlVo/MuxyQ6WryMAM7IODMegByiikE iJCArKoCai9sXprvz8TaenbKqTgy0DSlkhNgWe1e/z+Y3COz5wAU3k5617xdAf/DPBbj n/BNIv7JcBdhVmuB/o2ITbABIQ8gAx2jNtPH9vOPU+TJ+Z0g9EQHPoEtWBHmzXdV280Y jlIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=+rsli49ymfYIDDcZJ/yXC1Y4r08cVD11liWxP5aLvvE=; b=TLygblKVLDMJhIxsBiUY8CpkviBpVwl4iteTtGm6LnzlyeVelE9zlsAVABUliOs6bp ST4MT2z8b80aZ4VBOhMeaM7W1eIWivpxjWxLkklq78x0aEoukYCRdmLeWsgEWBuUqQRv Xj0Vd9PF+HM2GhLRL7FBZbChyikIvOW+GWhs90dwlOqU6RZ8osdwHzFLjh+d0rbFt4o9 uvCEVKAOqUZpkRoXxTSCgO0Y81V7Y1N5QrRGnTszSbcKqdpUpZ27id0M4J0iAEsh51Cz Trfp+N0fKZcgMqUU8LgG6bLWWlrPptWyupnsxQAQE2eRmuAuX0RGN9PiKBxmKnoXiCRc 3FbQ== X-Gm-Message-State: AHPjjUiHtFxKK0B2ElTRhRcfMTimPmwlyETB1QduKRVBctJreKCHsZG+ ksVjXoUEc1C+3rbh4tw= X-Google-Smtp-Source: ADKCNb5tS/vKPQNLw9hwCA490qlqtctaFz+c+jDJqTaA1koWQmN5OGHdoTPFVZ0tbhZURd/RAelgGg== X-Received: by 10.223.153.182 with SMTP id y51mr160421wrb.56.1504253280588; Fri, 01 Sep 2017 01:08:00 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 201sm1862866wmm.39.2017.09.01.01.07.59 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 01 Sep 2017 01:07:59 -0700 (PDT) From: Adrien Mazarguil To: dev@dpdk.org Date: Fri, 1 Sep 2017 10:06:45 +0200 Message-Id: X-Mailer: git-send-email 2.1.4 In-Reply-To: References: Subject: [dpdk-dev] [PATCH v2 30/51] net/mlx4: remove control path locks 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: Fri, 01 Sep 2017 08:08:01 -0000 Concurrent use of various control path functions (e.g. configuring a queue and destroying it simultaneously) may lead to undefined behavior. PMD are not supposed to protect themselves from misbehaving applications, and mlx4 is one of the few with internal locks on most control path operations. This adds unnecessary complexity. Leave this role to wrapper functions in ethdev. Signed-off-by: Adrien Mazarguil --- drivers/net/mlx4/mlx4.c | 90 +++------------------------------------ drivers/net/mlx4/mlx4.h | 4 -- drivers/net/mlx4/mlx4_flow.c | 15 +------ 3 files changed, 6 insertions(+), 103 deletions(-) diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index 80b0e3b..dc8a96f 100644 --- a/drivers/net/mlx4/mlx4.c +++ b/drivers/net/mlx4/mlx4.c @@ -59,7 +59,6 @@ #include #include #include -#include #include #include #include @@ -110,29 +109,6 @@ priv_rx_intr_vec_enable(struct priv *priv); static void priv_rx_intr_vec_disable(struct priv *priv); -/** - * Lock private structure to protect it from concurrent access in the - * control path. - * - * @param priv - * Pointer to private structure. - */ -void priv_lock(struct priv *priv) -{ - rte_spinlock_lock(&priv->lock); -} - -/** - * Unlock private structure. - * - * @param priv - * Pointer to private structure. - */ -void priv_unlock(struct priv *priv) -{ - rte_spinlock_unlock(&priv->lock); -} - /* Allocate a buffer on the stack and fill it with a printf format string. */ #define MKSTR(name, ...) \ char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \ @@ -559,13 +535,7 @@ dev_configure(struct rte_eth_dev *dev) static int mlx4_dev_configure(struct rte_eth_dev *dev) { - struct priv *priv = dev->data->dev_private; - int ret; - - priv_lock(priv); - ret = dev_configure(dev); - priv_unlock(priv); - return ret; + return dev_configure(dev); } static uint16_t mlx4_tx_burst(void *, struct rte_mbuf **, uint16_t); @@ -1317,14 +1287,12 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, struct txq *txq = (*priv->txqs)[idx]; int ret; - priv_lock(priv); DEBUG("%p: configuring queue %u for %u descriptors", (void *)dev, idx, desc); if (idx >= priv->txqs_n) { rte_errno = EOVERFLOW; ERROR("%p: queue index out of range (%u >= %u)", (void *)dev, idx, priv->txqs_n); - priv_unlock(priv); return -rte_errno; } if (txq != NULL) { @@ -1332,7 +1300,6 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, (void *)dev, idx, (void *)txq); if (priv->started) { rte_errno = EEXIST; - priv_unlock(priv); return -rte_errno; } (*priv->txqs)[idx] = NULL; @@ -1343,7 +1310,6 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, rte_errno = ENOMEM; ERROR("%p: unable to allocate queue index %u", (void *)dev, idx); - priv_unlock(priv); return -rte_errno; } } @@ -1358,7 +1324,6 @@ mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, /* Update send callback. */ dev->tx_pkt_burst = mlx4_tx_burst; } - priv_unlock(priv); return ret; } @@ -1378,7 +1343,6 @@ mlx4_tx_queue_release(void *dpdk_txq) if (txq == NULL) return; priv = txq->priv; - priv_lock(priv); for (i = 0; (i != priv->txqs_n); ++i) if ((*priv->txqs)[i] == txq) { DEBUG("%p: removing TX queue %p from list", @@ -1388,7 +1352,6 @@ mlx4_tx_queue_release(void *dpdk_txq) } txq_cleanup(txq); rte_free(txq); - priv_unlock(priv); } /* RX queues handling. */ @@ -1958,14 +1921,12 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, struct rxq *rxq = (*priv->rxqs)[idx]; int ret; - priv_lock(priv); DEBUG("%p: configuring queue %u for %u descriptors", (void *)dev, idx, desc); if (idx >= priv->rxqs_n) { rte_errno = EOVERFLOW; ERROR("%p: queue index out of range (%u >= %u)", (void *)dev, idx, priv->rxqs_n); - priv_unlock(priv); return -rte_errno; } if (rxq != NULL) { @@ -1973,7 +1934,6 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, (void *)dev, idx, (void *)rxq); if (priv->started) { rte_errno = EEXIST; - priv_unlock(priv); return -rte_errno; } (*priv->rxqs)[idx] = NULL; @@ -1986,7 +1946,6 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, rte_errno = ENOMEM; ERROR("%p: unable to allocate queue index %u", (void *)dev, idx); - priv_unlock(priv); return -rte_errno; } } @@ -2001,7 +1960,6 @@ mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc, /* Update receive callback. */ dev->rx_pkt_burst = mlx4_rx_burst; } - priv_unlock(priv); return ret; } @@ -2021,7 +1979,6 @@ mlx4_rx_queue_release(void *dpdk_rxq) if (rxq == NULL) return; priv = rxq->priv; - priv_lock(priv); for (i = 0; (i != priv->rxqs_n); ++i) if ((*priv->rxqs)[i] == rxq) { DEBUG("%p: removing RX queue %p from list", @@ -2033,7 +1990,6 @@ mlx4_rx_queue_release(void *dpdk_rxq) } rxq_cleanup(rxq); rte_free(rxq); - priv_unlock(priv); } static int @@ -2062,11 +2018,8 @@ mlx4_dev_start(struct rte_eth_dev *dev) struct priv *priv = dev->data->dev_private; int ret; - priv_lock(priv); - if (priv->started) { - priv_unlock(priv); + if (priv->started) return 0; - } DEBUG("%p: attaching configured flows to all RX queues", (void *)dev); priv->started = 1; ret = priv_mac_addr_add(priv); @@ -2096,13 +2049,11 @@ mlx4_dev_start(struct rte_eth_dev *dev) (void *)dev, strerror(ret)); goto err; } - priv_unlock(priv); return 0; err: /* Rollback. */ priv_mac_addr_del(priv); priv->started = 0; - priv_unlock(priv); return ret; } @@ -2119,16 +2070,12 @@ mlx4_dev_stop(struct rte_eth_dev *dev) { struct priv *priv = dev->data->dev_private; - priv_lock(priv); - if (!priv->started) { - priv_unlock(priv); + if (!priv->started) return; - } DEBUG("%p: detaching flows from all RX queues", (void *)dev); priv->started = 0; mlx4_priv_flow_stop(priv); priv_mac_addr_del(priv); - priv_unlock(priv); } /** @@ -2208,7 +2155,6 @@ mlx4_dev_close(struct rte_eth_dev *dev) if (priv == NULL) return; - priv_lock(priv); DEBUG("%p: closing device \"%s\"", (void *)dev, ((priv->ctx != NULL) ? priv->ctx->device->name : "")); @@ -2257,7 +2203,6 @@ mlx4_dev_close(struct rte_eth_dev *dev) priv_dev_removal_interrupt_handler_uninstall(priv, dev); priv_dev_link_interrupt_handler_uninstall(priv, dev); priv_rx_intr_vec_disable(priv); - priv_unlock(priv); memset(priv, 0, sizeof(*priv)); } @@ -2306,12 +2251,8 @@ static int mlx4_set_link_down(struct rte_eth_dev *dev) { struct priv *priv = dev->data->dev_private; - int err; - priv_lock(priv); - err = priv_set_link(priv, 0); - priv_unlock(priv); - return err; + return priv_set_link(priv, 0); } /** @@ -2327,12 +2268,8 @@ static int mlx4_set_link_up(struct rte_eth_dev *dev) { struct priv *priv = dev->data->dev_private; - int err; - priv_lock(priv); - err = priv_set_link(priv, 1); - priv_unlock(priv); - return err; + return priv_set_link(priv, 1); } /** @@ -2353,7 +2290,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info) info->pci_dev = RTE_ETH_DEV_TO_PCI(dev); if (priv == NULL) return; - priv_lock(priv); /* FIXME: we should ask the device for these values. */ info->min_rx_bufsize = 32; info->max_rx_pktlen = 65536; @@ -2380,7 +2316,6 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info) ETH_LINK_SPEED_20G | ETH_LINK_SPEED_40G | ETH_LINK_SPEED_56G; - priv_unlock(priv); } /** @@ -2401,7 +2336,6 @@ mlx4_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) if (priv == NULL) return; - priv_lock(priv); /* Add software counters. */ for (i = 0; (i != priv->rxqs_n); ++i) { struct rxq *rxq = (*priv->rxqs)[i]; @@ -2436,7 +2370,6 @@ mlx4_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) tmp.oerrors += txq->stats.odropped; } *stats = tmp; - priv_unlock(priv); } /** @@ -2454,7 +2387,6 @@ mlx4_stats_reset(struct rte_eth_dev *dev) if (priv == NULL) return; - priv_lock(priv); for (i = 0; (i != priv->rxqs_n); ++i) { if ((*priv->rxqs)[i] == NULL) continue; @@ -2469,7 +2401,6 @@ mlx4_stats_reset(struct rte_eth_dev *dev) (*priv->txqs)[i]->stats = (struct mlx4_txq_stats){ .idx = idx }; } - priv_unlock(priv); } /** @@ -2494,7 +2425,6 @@ mlx4_link_update(struct rte_eth_dev *dev, int wait_to_complete) struct rte_eth_link dev_link; int link_speed = 0; - /* priv_lock() is not taken to allow concurrent calls. */ if (priv == NULL) { rte_errno = EINVAL; return -rte_errno; @@ -2543,7 +2473,6 @@ mlx4_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu) struct priv *priv = dev->data->dev_private; int ret = 0; - priv_lock(priv); /* Set kernel interface MTU first. */ if (priv_set_mtu(priv, mtu)) { ret = rte_errno; @@ -2554,7 +2483,6 @@ mlx4_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu) DEBUG("adapter port %u MTU set to %u", priv->port, mtu); priv->mtu = mtu; out: - priv_unlock(priv); assert(ret >= 0); return -ret; } @@ -2581,7 +2509,6 @@ mlx4_dev_get_flow_ctrl(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf) int ret; ifr.ifr_data = (void *)ðpause; - priv_lock(priv); if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) { ret = rte_errno; WARN("ioctl(SIOCETHTOOL, ETHTOOL_GPAUSEPARAM)" @@ -2600,7 +2527,6 @@ mlx4_dev_get_flow_ctrl(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf) fc_conf->mode = RTE_FC_NONE; ret = 0; out: - priv_unlock(priv); assert(ret >= 0); return -ret; } @@ -2638,7 +2564,6 @@ mlx4_dev_set_flow_ctrl(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf) ethpause.tx_pause = 1; else ethpause.tx_pause = 0; - priv_lock(priv); if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) { ret = rte_errno; WARN("ioctl(SIOCETHTOOL, ETHTOOL_SPAUSEPARAM)" @@ -2648,7 +2573,6 @@ mlx4_dev_set_flow_ctrl(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf) } ret = 0; out: - priv_unlock(priv); assert(ret >= 0); return -ret; } @@ -2874,11 +2798,9 @@ mlx4_dev_link_status_handler(void *arg) uint32_t events; int ret; - priv_lock(priv); assert(priv->pending_alarm == 1); priv->pending_alarm = 0; ret = priv_dev_status_handler(priv, dev, &events); - priv_unlock(priv); if (ret > 0 && events & (1 << RTE_ETH_EVENT_INTR_LSC)) _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL, NULL); @@ -2901,9 +2823,7 @@ mlx4_dev_interrupt_handler(void *cb_arg) uint32_t ev; int i; - priv_lock(priv); ret = priv_dev_status_handler(priv, dev, &ev); - priv_unlock(priv); if (ret > 0) { for (i = RTE_ETH_EVENT_UNKNOWN; i < RTE_ETH_EVENT_MAX; diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index c619c87..c840e27 100644 --- a/drivers/net/mlx4/mlx4.h +++ b/drivers/net/mlx4/mlx4.h @@ -229,10 +229,6 @@ struct priv { struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */ LIST_HEAD(mlx4_flows, rte_flow) flows; struct rte_intr_conf intr_conf; /* Active interrupt configuration. */ - rte_spinlock_t lock; /* Lock for control functions. */ }; -void priv_lock(struct priv *priv); -void priv_unlock(struct priv *priv); - #endif /* RTE_PMD_MLX4_H_ */ diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c index 7dcb059..07305f1 100644 --- a/drivers/net/mlx4/mlx4_flow.c +++ b/drivers/net/mlx4/mlx4_flow.c @@ -703,13 +703,9 @@ mlx4_flow_validate(struct rte_eth_dev *dev, struct rte_flow_error *error) { struct priv *priv = dev->data->dev_private; - int ret; struct mlx4_flow flow = { .offset = sizeof(struct ibv_flow_attr) }; - priv_lock(priv); - ret = priv_flow_validate(priv, attr, items, actions, error, &flow); - priv_unlock(priv); - return ret; + return priv_flow_validate(priv, attr, items, actions, error, &flow); } /** @@ -936,13 +932,11 @@ mlx4_flow_create(struct rte_eth_dev *dev, struct priv *priv = dev->data->dev_private; struct rte_flow *flow; - priv_lock(priv); flow = priv_flow_create(priv, attr, items, actions, error); if (flow) { LIST_INSERT_HEAD(&priv->flows, flow, next); DEBUG("Flow created %p", (void *)flow); } - priv_unlock(priv); return flow; } @@ -969,17 +963,14 @@ mlx4_flow_isolate(struct rte_eth_dev *dev, { struct priv *priv = dev->data->dev_private; - priv_lock(priv); if (priv->rxqs) { rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL, "isolated mode must be set" " before configuring the device"); - priv_unlock(priv); return -rte_errno; } priv->isolated = !!enable; - priv_unlock(priv); return 0; } @@ -1017,9 +1008,7 @@ mlx4_flow_destroy(struct rte_eth_dev *dev, struct priv *priv = dev->data->dev_private; (void)error; - priv_lock(priv); priv_flow_destroy(priv, flow); - priv_unlock(priv); return 0; } @@ -1053,9 +1042,7 @@ mlx4_flow_flush(struct rte_eth_dev *dev, struct priv *priv = dev->data->dev_private; (void)error; - priv_lock(priv); priv_flow_flush(priv); - priv_unlock(priv); return 0; } -- 2.1.4