Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/91711/?format=api
{ "id": 91711, "url": "http://patches.dpdk.org/api/patches/91711/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/patch/1618653299-40380-7-git-send-email-humin29@huawei.com/", "project": { "id": 1, "url": "http://patches.dpdk.org/api/projects/1/?format=api", "name": "DPDK", "link_name": "dpdk", "list_id": "dev.dpdk.org", "list_email": "dev@dpdk.org", "web_url": "http://core.dpdk.org", "scm_url": "git://dpdk.org/dpdk", "webscm_url": "http://git.dpdk.org/dpdk", "list_archive_url": "https://inbox.dpdk.org/dev", "list_archive_url_format": "https://inbox.dpdk.org/dev/{}", "commit_url_format": "" }, "msgid": "<1618653299-40380-7-git-send-email-humin29@huawei.com>", "list_archive_url": "https://inbox.dpdk.org/dev/1618653299-40380-7-git-send-email-humin29@huawei.com", "date": "2021-04-17T09:54:58", "name": "[6/7] net/hns3: fix FDIR lock bug", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": true, "hash": "dca9c7aa34b4bf3061d84dceb09fa095395ee566", "submitter": { "id": 1944, "url": "http://patches.dpdk.org/api/people/1944/?format=api", "name": "humin (Q)", "email": "humin29@huawei.com" }, "delegate": { "id": 319, "url": "http://patches.dpdk.org/api/users/319/?format=api", "username": "fyigit", "first_name": "Ferruh", "last_name": "Yigit", "email": "ferruh.yigit@amd.com" }, "mbox": "http://patches.dpdk.org/project/dpdk/patch/1618653299-40380-7-git-send-email-humin29@huawei.com/mbox/", "series": [ { "id": 16463, "url": "http://patches.dpdk.org/api/series/16463/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/list/?series=16463", "date": "2021-04-17T09:54:52", "name": "features and bugfix for hns3 PMD", "version": 1, "mbox": "http://patches.dpdk.org/series/16463/mbox/" } ], "comments": "http://patches.dpdk.org/api/patches/91711/comments/", "check": "success", "checks": "http://patches.dpdk.org/api/patches/91711/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@inbox.dpdk.org", "Delivered-To": "patchwork@inbox.dpdk.org", "Received": [ "from mails.dpdk.org (mails.dpdk.org [217.70.189.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id 751D3A0562;\n\tSat, 17 Apr 2021 11:55:27 +0200 (CEST)", "from [217.70.189.124] (localhost [127.0.0.1])\n\tby mails.dpdk.org (Postfix) with ESMTP id 8D5F5161F13;\n\tSat, 17 Apr 2021 11:54:59 +0200 (CEST)", "from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190])\n by mails.dpdk.org (Postfix) with ESMTP id CACAD4068F\n for <dev@dpdk.org>; Sat, 17 Apr 2021 11:54:51 +0200 (CEST)", "from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.59])\n by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FMpK90kH0z17R8Y\n for <dev@dpdk.org>; Sat, 17 Apr 2021 17:52:29 +0800 (CST)", "from localhost.localdomain (10.69.192.56) by\n DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id\n 14.3.498.0; Sat, 17 Apr 2021 17:54:46 +0800" ], "From": "\"Min Hu (Connor)\" <humin29@huawei.com>", "To": "<dev@dpdk.org>", "CC": "<ferruh.yigit@intel.com>", "Date": "Sat, 17 Apr 2021 17:54:58 +0800", "Message-ID": "<1618653299-40380-7-git-send-email-humin29@huawei.com>", "X-Mailer": "git-send-email 2.7.4", "In-Reply-To": "<1618653299-40380-1-git-send-email-humin29@huawei.com>", "References": "<1618653299-40380-1-git-send-email-humin29@huawei.com>", "MIME-Version": "1.0", "Content-Type": "text/plain", "X-Originating-IP": "[10.69.192.56]", "X-CFilter-Loop": "Reflected", "Subject": "[dpdk-dev] [PATCH 6/7] net/hns3: fix FDIR lock bug", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n <mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "content": "From: Chengwen Feng <fengchengwen@huawei.com>\n\nCurrently, the fdir lock was used to protect concurrent access in\nmultiple processes, it has the following problems:\n1) Lack of protection for fdir reset recover.\n2) Only part data is protected, eg. the filterlist is not protected.\n\nWe use the following scheme:\n1) Del the fdir lock.\n2) Add a flow lock and provides rte flow driver ops API-level\nprotection.\n3) Declare support RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE.\n\nFixes: fcba820d9b9e (\"net/hns3: support flow director\")\nCc: stable@dpdk.org\n\nSigned-off-by: Chengwen Feng <fengchengwen@huawei.com>\nSigned-off-by: Min Hu (Connor) <humin29@huawei.com>\n---\n drivers/net/hns3/hns3_ethdev.c | 4 +-\n drivers/net/hns3/hns3_ethdev.h | 4 ++\n drivers/net/hns3/hns3_ethdev_vf.c | 3 +-\n drivers/net/hns3/hns3_fdir.c | 28 ++++++------\n drivers/net/hns3/hns3_fdir.h | 3 +-\n drivers/net/hns3/hns3_flow.c | 96 ++++++++++++++++++++++++++++++++++++---\n 6 files changed, 111 insertions(+), 27 deletions(-)", "diff": "diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c\nindex c276c6b..bcbebd0 100644\n--- a/drivers/net/hns3/hns3_ethdev.c\n+++ b/drivers/net/hns3/hns3_ethdev.c\n@@ -7334,8 +7334,8 @@ hns3_dev_init(struct rte_eth_dev *eth_dev)\n \t\tPMD_INIT_LOG(ERR, \"Failed to alloc memory for process private\");\n \t\treturn -ENOMEM;\n \t}\n-\t/* initialize flow filter lists */\n-\thns3_filterlist_init(eth_dev);\n+\n+\thns3_flow_init(eth_dev);\n \n \thns3_set_rxtx_function(eth_dev);\n \teth_dev->dev_ops = &hns3_eth_dev_ops;\ndiff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h\nindex 7b7d359..b1360cb 100644\n--- a/drivers/net/hns3/hns3_ethdev.h\n+++ b/drivers/net/hns3/hns3_ethdev.h\n@@ -5,6 +5,7 @@\n #ifndef _HNS3_ETHDEV_H_\n #define _HNS3_ETHDEV_H_\n \n+#include <pthread.h>\n #include <sys/time.h>\n #include <ethdev_driver.h>\n #include <rte_byteorder.h>\n@@ -613,6 +614,9 @@ struct hns3_hw {\n \tuint8_t udp_cksum_mode;\n \n \tstruct hns3_port_base_vlan_config port_base_vlan_cfg;\n+\n+\tpthread_mutex_t flows_lock; /* rte_flow ops lock */\n+\n \t/*\n \t * PMD setup and configuration is not thread safe. Since it is not\n \t * performance sensitive, it is better to guarantee thread-safety\ndiff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c\nindex 58809fb..cf2b74e 100644\n--- a/drivers/net/hns3/hns3_ethdev_vf.c\n+++ b/drivers/net/hns3/hns3_ethdev_vf.c\n@@ -2910,8 +2910,7 @@ hns3vf_dev_init(struct rte_eth_dev *eth_dev)\n \t\treturn -ENOMEM;\n \t}\n \n-\t/* initialize flow filter lists */\n-\thns3_filterlist_init(eth_dev);\n+\thns3_flow_init(eth_dev);\n \n \thns3_set_rxtx_function(eth_dev);\n \teth_dev->dev_ops = &hns3vf_eth_dev_ops;\ndiff --git a/drivers/net/hns3/hns3_fdir.c b/drivers/net/hns3/hns3_fdir.c\nindex 603cc82..87c1aef 100644\n--- a/drivers/net/hns3/hns3_fdir.c\n+++ b/drivers/net/hns3/hns3_fdir.c\n@@ -830,7 +830,6 @@ int hns3_fdir_filter_init(struct hns3_adapter *hns)\n \n \tfdir_hash_params.socket_id = rte_socket_id();\n \tTAILQ_INIT(&fdir_info->fdir_list);\n-\trte_spinlock_init(&fdir_info->flows_lock);\n \tsnprintf(fdir_hash_name, RTE_HASH_NAMESIZE, \"%s\", hns->hw.data->name);\n \tfdir_info->hash_handle = rte_hash_create(&fdir_hash_params);\n \tif (fdir_info->hash_handle == NULL) {\n@@ -856,7 +855,6 @@ void hns3_fdir_filter_uninit(struct hns3_adapter *hns)\n \tstruct hns3_fdir_info *fdir_info = &pf->fdir;\n \tstruct hns3_fdir_rule_ele *fdir_filter;\n \n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \tif (fdir_info->hash_map) {\n \t\trte_free(fdir_info->hash_map);\n \t\tfdir_info->hash_map = NULL;\n@@ -865,7 +863,6 @@ void hns3_fdir_filter_uninit(struct hns3_adapter *hns)\n \t\trte_hash_free(fdir_info->hash_handle);\n \t\tfdir_info->hash_handle = NULL;\n \t}\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \n \tfdir_filter = TAILQ_FIRST(&fdir_info->fdir_list);\n \twhile (fdir_filter) {\n@@ -891,10 +888,8 @@ static int hns3_fdir_filter_lookup(struct hns3_fdir_info *fdir_info,\n \thash_sig_t sig;\n \tint ret;\n \n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \tsig = rte_hash_crc(key, sizeof(*key), 0);\n \tret = rte_hash_lookup_with_hash(fdir_info->hash_handle, key, sig);\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \n \treturn ret;\n }\n@@ -908,11 +903,9 @@ static int hns3_insert_fdir_filter(struct hns3_hw *hw,\n \tint ret;\n \n \tkey = &fdir_filter->fdir_conf.key_conf;\n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \tsig = rte_hash_crc(key, sizeof(*key), 0);\n \tret = rte_hash_add_key_with_hash(fdir_info->hash_handle, key, sig);\n \tif (ret < 0) {\n-\t\trte_spinlock_unlock(&fdir_info->flows_lock);\n \t\thns3_err(hw, \"Hash table full? err:%d(%s)!\", ret,\n \t\t\t strerror(-ret));\n \t\treturn ret;\n@@ -920,7 +913,6 @@ static int hns3_insert_fdir_filter(struct hns3_hw *hw,\n \n \tfdir_info->hash_map[ret] = fdir_filter;\n \tTAILQ_INSERT_TAIL(&fdir_info->fdir_list, fdir_filter, entries);\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \n \treturn ret;\n }\n@@ -933,11 +925,9 @@ static int hns3_remove_fdir_filter(struct hns3_hw *hw,\n \thash_sig_t sig;\n \tint ret;\n \n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \tsig = rte_hash_crc(key, sizeof(*key), 0);\n \tret = rte_hash_del_key_with_hash(fdir_info->hash_handle, key, sig);\n \tif (ret < 0) {\n-\t\trte_spinlock_unlock(&fdir_info->flows_lock);\n \t\thns3_err(hw, \"Delete hash key fail ret=%d\", ret);\n \t\treturn ret;\n \t}\n@@ -945,7 +935,6 @@ static int hns3_remove_fdir_filter(struct hns3_hw *hw,\n \tfdir_filter = fdir_info->hash_map[ret];\n \tfdir_info->hash_map[ret] = NULL;\n \tTAILQ_REMOVE(&fdir_info->fdir_list, fdir_filter, entries);\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \n \trte_free(fdir_filter);\n \n@@ -1000,11 +989,9 @@ int hns3_fdir_filter_program(struct hns3_adapter *hns,\n \trule->location = ret;\n \tnode->fdir_conf.location = ret;\n \n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \tret = hns3_config_action(hw, rule);\n \tif (!ret)\n \t\tret = hns3_config_key(hns, rule);\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \tif (ret) {\n \t\thns3_err(hw, \"Failed to config fdir: %u src_ip:%x dst_ip:%x \"\n \t\t\t \"src_port:%u dst_port:%u ret = %d\",\n@@ -1029,9 +1016,7 @@ int hns3_clear_all_fdir_filter(struct hns3_adapter *hns)\n \tint ret = 0;\n \n \t/* flush flow director */\n-\trte_spinlock_lock(&fdir_info->flows_lock);\n \trte_hash_reset(fdir_info->hash_handle);\n-\trte_spinlock_unlock(&fdir_info->flows_lock);\n \n \tfdir_filter = TAILQ_FIRST(&fdir_info->fdir_list);\n \twhile (fdir_filter) {\n@@ -1059,6 +1044,17 @@ int hns3_restore_all_fdir_filter(struct hns3_adapter *hns)\n \tbool err = false;\n \tint ret;\n \n+\t/*\n+\t * This API is called in the reset recovery process, the parent function\n+\t * must hold hw->lock.\n+\t * There maybe deadlock if acquire hw->flows_lock directly because rte\n+\t * flow driver ops first acquire hw->flows_lock and then may acquire\n+\t * hw->lock.\n+\t * So here first release the hw->lock and then acquire the\n+\t * hw->flows_lock to avoid deadlock.\n+\t */\n+\trte_spinlock_unlock(&hw->lock);\n+\tpthread_mutex_lock(&hw->flows_lock);\n \tTAILQ_FOREACH(fdir_filter, &fdir_info->fdir_list, entries) {\n \t\tret = hns3_config_action(hw, &fdir_filter->fdir_conf);\n \t\tif (!ret)\n@@ -1069,6 +1065,8 @@ int hns3_restore_all_fdir_filter(struct hns3_adapter *hns)\n \t\t\t\tbreak;\n \t\t}\n \t}\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\trte_spinlock_lock(&hw->lock);\n \n \tif (err) {\n \t\thns3_err(hw, \"Fail to restore FDIR filter, ret = %d\", ret);\ndiff --git a/drivers/net/hns3/hns3_fdir.h b/drivers/net/hns3/hns3_fdir.h\nindex 266d37c..fc62daa 100644\n--- a/drivers/net/hns3/hns3_fdir.h\n+++ b/drivers/net/hns3/hns3_fdir.h\n@@ -199,7 +199,6 @@ struct hns3_process_private {\n * A structure used to define fields of a FDIR related info.\n */\n struct hns3_fdir_info {\n-\trte_spinlock_t flows_lock;\n \tstruct hns3_fdir_rule_list fdir_list;\n \tstruct hns3_fdir_rule_ele **hash_map;\n \tstruct rte_hash *hash_handle;\n@@ -220,7 +219,7 @@ int hns3_fdir_filter_program(struct hns3_adapter *hns,\n \t\t\t struct hns3_fdir_rule *rule, bool del);\n int hns3_clear_all_fdir_filter(struct hns3_adapter *hns);\n int hns3_get_count(struct hns3_hw *hw, uint32_t id, uint64_t *value);\n-void hns3_filterlist_init(struct rte_eth_dev *dev);\n+void hns3_flow_init(struct rte_eth_dev *dev);\n int hns3_restore_all_fdir_filter(struct hns3_adapter *hns);\n \n #endif /* _HNS3_FDIR_H_ */\ndiff --git a/drivers/net/hns3/hns3_flow.c b/drivers/net/hns3/hns3_flow.c\nindex 098b191..4511a49 100644\n--- a/drivers/net/hns3/hns3_flow.c\n+++ b/drivers/net/hns3/hns3_flow.c\n@@ -1214,9 +1214,18 @@ hns3_parse_fdir_filter(struct rte_eth_dev *dev,\n }\n \n void\n-hns3_filterlist_init(struct rte_eth_dev *dev)\n+hns3_flow_init(struct rte_eth_dev *dev)\n {\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n \tstruct hns3_process_private *process_list = dev->process_private;\n+\tpthread_mutexattr_t attr;\n+\n+\tif (rte_eal_process_type() == RTE_PROC_PRIMARY) {\n+\t\tpthread_mutexattr_init(&attr);\n+\t\tpthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);\n+\t\tpthread_mutex_init(&hw->flows_lock, &attr);\n+\t\tdev->data->dev_flags |= RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE;\n+\t}\n \n \tTAILQ_INIT(&process_list->fdir_list);\n \tTAILQ_INIT(&process_list->filter_rss_list);\n@@ -2002,12 +2011,87 @@ hns3_flow_query(struct rte_eth_dev *dev, struct rte_flow *flow,\n \treturn 0;\n }\n \n+static int\n+hns3_flow_validate_wrap(struct rte_eth_dev *dev,\n+\t\t\tconst struct rte_flow_attr *attr,\n+\t\t\tconst struct rte_flow_item pattern[],\n+\t\t\tconst struct rte_flow_action actions[],\n+\t\t\tstruct rte_flow_error *error)\n+{\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n+\tint ret;\n+\n+\tpthread_mutex_lock(&hw->flows_lock);\n+\tret = hns3_flow_validate(dev, attr, pattern, actions, error);\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\n+\treturn ret;\n+}\n+\n+static struct rte_flow *\n+hns3_flow_create_wrap(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,\n+\t\t const struct rte_flow_item pattern[],\n+\t\t const struct rte_flow_action actions[],\n+\t\t struct rte_flow_error *error)\n+{\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n+\tstruct rte_flow *flow;\n+\n+\tpthread_mutex_lock(&hw->flows_lock);\n+\tflow = hns3_flow_create(dev, attr, pattern, actions, error);\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\n+\treturn flow;\n+}\n+\n+static int\n+hns3_flow_destroy_wrap(struct rte_eth_dev *dev, struct rte_flow *flow,\n+\t\t struct rte_flow_error *error)\n+{\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n+\tint ret;\n+\n+\tpthread_mutex_lock(&hw->flows_lock);\n+\tret = hns3_flow_destroy(dev, flow, error);\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\n+\treturn ret;\n+}\n+\n+static int\n+hns3_flow_flush_wrap(struct rte_eth_dev *dev, struct rte_flow_error *error)\n+{\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n+\tint ret;\n+\n+\tpthread_mutex_lock(&hw->flows_lock);\n+\tret = hns3_flow_flush(dev, error);\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\n+\treturn ret;\n+}\n+\n+static int\n+hns3_flow_query_wrap(struct rte_eth_dev *dev, struct rte_flow *flow,\n+\t\t const struct rte_flow_action *actions, void *data,\n+\t\t struct rte_flow_error *error)\n+{\n+\tstruct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private);\n+\tint ret;\n+\n+\tpthread_mutex_lock(&hw->flows_lock);\n+\tret = hns3_flow_query(dev, flow, actions, data, error);\n+\tpthread_mutex_unlock(&hw->flows_lock);\n+\n+\treturn ret;\n+}\n+\n static const struct rte_flow_ops hns3_flow_ops = {\n-\t.validate = hns3_flow_validate,\n-\t.create = hns3_flow_create,\n-\t.destroy = hns3_flow_destroy,\n-\t.flush = hns3_flow_flush,\n-\t.query = hns3_flow_query,\n+\t.validate = hns3_flow_validate_wrap,\n+\t.create = hns3_flow_create_wrap,\n+\t.destroy = hns3_flow_destroy_wrap,\n+\t.flush = hns3_flow_flush_wrap,\n+\t.query = hns3_flow_query_wrap,\n \t.isolate = NULL,\n };\n \n", "prefixes": [ "6/7" ] }