DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v3] net/ice: remove unnecessary variable
@ 2020-03-04  2:05 Qi Zhang
  2020-03-04  3:02 ` Ye Xiaolong
  0 siblings, 1 reply; 2+ messages in thread
From: Qi Zhang @ 2020-03-04  2:05 UTC (permalink / raw)
  To: xiaolong.ye, yahui.cao, simei.su, wei.zhao; +Cc: dev, Qi Zhang, stable

Remove unnecessary variable "meta" in ice_flow_create and
ice_flow_validate, it should be defined when really be needed:
its ice_parse_engine_create and ice_parse_engine_validate.

A meta data be created by parser->parse_pattern_action should be
freed inside parser->engine->create.

During a validate operation, since parser->engine_>create will not
be invoked, a meta is not necessary be created during parser, so
NULL will parsed to engine->parse_pattern_action and all parser's
parse_pattern_action need to be modified to handle meta = NULL
properly.

With above implementation, the patch also fixes a potentional memory
leak in ice_parse_engine_validate, since meta may not be freed.

BTW, an engine without a create op should be regarded as a bug. So
use RTE_ASSERT to replace runtime engine->create == NULL check in
ice_parse_engine_create.

Fixes: 4e27d3ed02bd ("net/ice: fix flow API framework")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---

v3:
- rollback meta free in ice_switch_create and ice_hash_create, since we
  assume parser->parse_pattern_action and parser->engine->create should
  be paired for meta memory allocation / free.
- free sw_meta_ptr and list if meta == NULL in ice_switch_parse_pattern_action
- use RTE_ASSERT to replace runtime check for engine->create == NULL;

v2:
- remove meta free from low level engine.
- alloc parse NULL to parser->parse_pattern_action.

 drivers/net/ice/ice_fdir_filter.c   |  3 ++-
 drivers/net/ice/ice_generic_flow.c  | 32 +++++++++++---------------------
 drivers/net/ice/ice_hash.c          | 11 +++++++----
 drivers/net/ice/ice_switch_filter.c | 16 +++++++++++-----
 4 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c
index 5a791610f..6342b560c 100644
--- a/drivers/net/ice/ice_fdir_filter.c
+++ b/drivers/net/ice/ice_fdir_filter.c
@@ -1966,7 +1966,8 @@ ice_fdir_parse(struct ice_adapter *ad,
 	if (ret)
 		goto error;
 
-	*meta = filter;
+	if (meta)
+		*meta = filter;
 error:
 	rte_free(item);
 	return ret;
diff --git a/drivers/net/ice/ice_generic_flow.c b/drivers/net/ice/ice_generic_flow.c
index 38ac799d8..64c0543c0 100644
--- a/drivers/net/ice/ice_generic_flow.c
+++ b/drivers/net/ice/ice_generic_flow.c
@@ -1375,7 +1375,6 @@ typedef struct ice_flow_engine * (*parse_engine_t)(struct ice_adapter *ad,
 		struct ice_parser_list *parser_list,
 		const struct rte_flow_item pattern[],
 		const struct rte_flow_action actions[],
-		void **meta,
 		struct rte_flow_error *error);
 
 void
@@ -1713,11 +1712,11 @@ ice_parse_engine_create(struct ice_adapter *ad,
 		struct ice_parser_list *parser_list,
 		const struct rte_flow_item pattern[],
 		const struct rte_flow_action actions[],
-		void **meta,
 		struct rte_flow_error *error)
 {
 	struct ice_flow_engine *engine = NULL;
 	struct ice_flow_parser_node *parser_node;
+	void *meta = NULL;
 	void *temp;
 
 	TAILQ_FOREACH_SAFE(parser_node, parser_list, node, temp) {
@@ -1726,18 +1725,12 @@ ice_parse_engine_create(struct ice_adapter *ad,
 		if (parser_node->parser->parse_pattern_action(ad,
 				parser_node->parser->array,
 				parser_node->parser->array_len,
-				pattern, actions, meta, error) < 0)
+				pattern, actions, &meta, error) < 0)
 			continue;
 
 		engine = parser_node->parser->engine;
-		if (engine->create == NULL) {
-			rte_flow_error_set(error, EINVAL,
-					RTE_FLOW_ERROR_TYPE_HANDLE,
-					NULL, "Invalid engine");
-			continue;
-		}
-
-		ret = engine->create(ad, flow, *meta, error);
+		RTE_ASSERT(engine->create != NULL);
+		ret = engine->create(ad, flow, meta, error);
 		if (ret == 0)
 			return engine;
 		else if (ret == -EEXIST)
@@ -1752,7 +1745,6 @@ ice_parse_engine_validate(struct ice_adapter *ad,
 		struct ice_parser_list *parser_list,
 		const struct rte_flow_item pattern[],
 		const struct rte_flow_action actions[],
-		void **meta,
 		struct rte_flow_error *error)
 {
 	struct ice_flow_engine *engine = NULL;
@@ -1763,12 +1755,13 @@ ice_parse_engine_validate(struct ice_adapter *ad,
 		if (parser_node->parser->parse_pattern_action(ad,
 				parser_node->parser->array,
 				parser_node->parser->array_len,
-				pattern, actions, meta, error) < 0)
+				pattern, actions, NULL, error) < 0)
 			continue;
 
 		engine = parser_node->parser->engine;
 		break;
 	}
+
 	return engine;
 }
 
@@ -1779,7 +1772,6 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
 		const struct rte_flow_item pattern[],
 		const struct rte_flow_action actions[],
 		struct ice_flow_engine **engine,
-		void **meta,
 		parse_engine_t ice_parse_engine,
 		struct rte_flow_error *error)
 {
@@ -1814,7 +1806,7 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
 		return ret;
 
 	*engine = ice_parse_engine(ad, flow, &pf->rss_parser_list,
-			pattern, actions, meta, error);
+			pattern, actions, error);
 	if (*engine != NULL)
 		return 0;
 
@@ -1822,11 +1814,11 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
 	case ICE_FLOW_CLASSIFY_STAGE_DISTRIBUTOR_ONLY:
 	case ICE_FLOW_CLASSIFY_STAGE_DISTRIBUTOR:
 		*engine = ice_parse_engine(ad, flow, &pf->dist_parser_list,
-				pattern, actions, meta, error);
+				pattern, actions, error);
 		break;
 	case ICE_FLOW_CLASSIFY_STAGE_PERMISSION:
 		*engine = ice_parse_engine(ad, flow, &pf->perm_parser_list,
-				pattern, actions, meta, error);
+				pattern, actions, error);
 		break;
 	default:
 		return -EINVAL;
@@ -1845,11 +1837,10 @@ ice_flow_validate(struct rte_eth_dev *dev,
 		const struct rte_flow_action actions[],
 		struct rte_flow_error *error)
 {
-	void *meta;
 	struct ice_flow_engine *engine;
 
 	return ice_flow_process_filter(dev, NULL, attr, pattern, actions,
-			&engine, &meta, ice_parse_engine_validate, error);
+			&engine, ice_parse_engine_validate, error);
 }
 
 static struct rte_flow *
@@ -1863,7 +1854,6 @@ ice_flow_create(struct rte_eth_dev *dev,
 	struct rte_flow *flow = NULL;
 	int ret;
 	struct ice_flow_engine *engine = NULL;
-	void *meta;
 
 	flow = rte_zmalloc("ice_flow", sizeof(struct rte_flow), 0);
 	if (!flow) {
@@ -1874,7 +1864,7 @@ ice_flow_create(struct rte_eth_dev *dev,
 	}
 
 	ret = ice_flow_process_filter(dev, flow, attr, pattern, actions,
-			&engine, &meta, ice_parse_engine_create, error);
+			&engine, ice_parse_engine_create, error);
 	if (ret < 0)
 		goto free_flow;
 	flow->engine = engine;
diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index d891538bd..c692130e7 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -432,14 +432,17 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 		goto error;
 
 	/* Save protocol header to rss_meta. */
-	*meta = rss_meta_ptr;
-	((struct rss_meta *)*meta)->pkt_hdr = ((struct rss_type_match_hdr *)
+	rss_meta_ptr->pkt_hdr = ((struct rss_type_match_hdr *)
 		(pattern_match_item->meta))->hdr_mask;
 
 	/* Check rss action. */
-	ret = ice_hash_parse_action(pattern_match_item, actions, meta, error);
+	ret = ice_hash_parse_action(pattern_match_item, actions,
+				    (void **)&rss_meta_ptr, error);
+
 error:
-	if (ret)
+	if (!ret && meta)
+		*meta = rss_meta_ptr;
+	else
 		rte_free(rss_meta_ptr);
 	rte_free(pattern_match_item);
 
diff --git a/drivers/net/ice/ice_switch_filter.c b/drivers/net/ice/ice_switch_filter.c
index 4a9356b31..87d484b75 100644
--- a/drivers/net/ice/ice_switch_filter.c
+++ b/drivers/net/ice/ice_switch_filter.c
@@ -255,7 +255,6 @@ ice_switch_create(struct ice_adapter *ad,
 error:
 	rte_free(list);
 	rte_free(meta);
-
 	return -rte_errno;
 }
 
@@ -1088,10 +1087,17 @@ ice_switch_parse_pattern_action(struct ice_adapter *ad,
 				   "Invalid input action");
 		goto error;
 	}
-	*meta = sw_meta_ptr;
-	((struct sw_meta *)*meta)->list = list;
-	((struct sw_meta *)*meta)->lkups_num = lkups_num;
-	((struct sw_meta *)*meta)->rule_info = rule_info;
+
+	if (meta) {
+		*meta = sw_meta_ptr;
+		((struct sw_meta *)*meta)->list = list;
+		((struct sw_meta *)*meta)->lkups_num = lkups_num;
+		((struct sw_meta *)*meta)->rule_info = rule_info;
+	} else {
+		rte_free(list);
+		rte_free(sw_meta_ptr);
+	}
+
 	rte_free(pattern_match_item);
 
 	return 0;
-- 
2.13.6


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-03-04  3:04 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04  2:05 [dpdk-dev] [PATCH v3] net/ice: remove unnecessary variable Qi Zhang
2020-03-04  3:02 ` Ye Xiaolong

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).