[dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable

[dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable

[Qi Zhang]

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.

In a validate opertion, a meta is not necessary be created during
parser, so NULL will be parsed to low level engine and all engine's
parse_pattern_action need to be modified properly.

The the patch also fix a potentional memory leak in
ice_parse_engine_validate, since meta may not be freed.

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

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

v2:
- remove meta free to low level engine
- allow parse meta = NULL to parse_pattern_action.

 drivers/net/ice/ice_fdir_filter.c   |  3 ++-
 drivers/net/ice/ice_generic_flow.c  | 24 ++++++++++--------------
 drivers/net/ice/ice_hash.c          | 13 +++++++------
 drivers/net/ice/ice_switch_filter.c | 14 +++++++-------
 4 files changed, 26 insertions(+), 28 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..6bcf28de8 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,7 +1725,7 @@ 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;
@@ -1737,7 +1736,7 @@ ice_parse_engine_create(struct ice_adapter *ad,
 			continue;
 		}
 
-		ret = engine->create(ad, flow, *meta, error);
+		ret = engine->create(ad, flow, meta, error);
 		if (ret == 0)
 			return engine;
 		else if (ret == -EEXIST)
@@ -1752,7 +1751,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 +1761,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 +1778,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 +1812,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 +1820,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 +1843,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 +1860,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 +1870,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..b37da9920 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);
 
@@ -503,12 +506,10 @@ ice_hash_create(struct ice_adapter *ad,
 
 out:
 	flow->rule = filter_ptr;
-	rte_free(meta);
 	return 0;
 
 error:
 	rte_free(filter_ptr);
-	rte_free(meta);
 	return -rte_errno;
 }
 
diff --git a/drivers/net/ice/ice_switch_filter.c b/drivers/net/ice/ice_switch_filter.c
index 4a9356b31..8a2ba8aa6 100644
--- a/drivers/net/ice/ice_switch_filter.c
+++ b/drivers/net/ice/ice_switch_filter.c
@@ -249,13 +249,10 @@ ice_switch_create(struct ice_adapter *ad,
 	}
 
 	rte_free(list);
-	rte_free(meta);
 	return 0;
 
 error:
 	rte_free(list);
-	rte_free(meta);
-
 	return -rte_errno;
 }
 
@@ -1088,10 +1085,13 @@ 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;
+	}
 	rte_free(pattern_match_item);
 
 	return 0;
-- 
2.13.6

Re: [dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable

[Ye Xiaolong]

Hi, Qi

On 03/03, Qi Zhang wrote:
>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.
>
>In a validate opertion, a meta is not necessary be created during
>parser, so NULL will be parsed to low level engine and all engine's
>parse_pattern_action need to be modified properly.
>
>The the patch also fix a potentional memory leak in

s/fix/fixes

>ice_parse_engine_validate, since meta may not be freed.
>
>Fixes: 4e27d3ed02bd ("net/ice: fix flow API framework")
>Cc: stable@dpdk.org
>
>Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>---
>
>v2:
>- remove meta free to low level engine
>- allow parse meta = NULL to parse_pattern_action.
>
> drivers/net/ice/ice_fdir_filter.c   |  3 ++-
> drivers/net/ice/ice_generic_flow.c  | 24 ++++++++++--------------
> drivers/net/ice/ice_hash.c          | 13 +++++++------
> drivers/net/ice/ice_switch_filter.c | 14 +++++++-------
> 4 files changed, 26 insertions(+), 28 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..6bcf28de8 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,7 +1725,7 @@ 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;
>@@ -1737,7 +1736,7 @@ ice_parse_engine_create(struct ice_adapter *ad,
> 			continue;
> 		}
> 
>-		ret = engine->create(ad, flow, *meta, error);
>+		ret = engine->create(ad, flow, meta, error);
> 		if (ret == 0)
> 			return engine;
> 		else if (ret == -EEXIST)
>@@ -1752,7 +1751,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 +1761,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;
> 	}
>+

Irrelevant change.

> 	return engine;
> }
> 
>@@ -1779,7 +1778,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 +1812,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 +1820,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 +1843,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 +1860,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 +1870,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..b37da9920 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);
> 
>@@ -503,12 +506,10 @@ ice_hash_create(struct ice_adapter *ad,
> 
> out:
> 	flow->rule = filter_ptr;
>-	rte_free(meta);
> 	return 0;
> 
> error:
> 	rte_free(filter_ptr);
>-	rte_free(meta);
> 	return -rte_errno;
> }
> 
>diff --git a/drivers/net/ice/ice_switch_filter.c b/drivers/net/ice/ice_switch_filter.c
>index 4a9356b31..8a2ba8aa6 100644
>--- a/drivers/net/ice/ice_switch_filter.c
>+++ b/drivers/net/ice/ice_switch_filter.c
>@@ -249,13 +249,10 @@ ice_switch_create(struct ice_adapter *ad,
> 	}
> 
> 	rte_free(list);
>-	rte_free(meta);
> 	return 0;
> 
> error:
> 	rte_free(list);
>-	rte_free(meta);
>-
> 	return -rte_errno;
> }
> 
>@@ -1088,10 +1085,13 @@ 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;
>+	}

If meta is NULL for ice_parse_engine_validate case, do we need free sw_meta_ptr
explicitly in switch like we free(rss_meta_ptr) in hash? And also free the list?


And for case meta is not NULL, who is responsible for freeing it eventually?

Thanks,
Xiaolong

> 	rte_free(pattern_match_item);
> 
> 	return 0;
>-- 
>2.13.6
>

Re: [dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Tuesday, March 3, 2020 4:27 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Cao, Yahui <yahui.cao@intel.com>; Su, Simei <simei.su@intel.com>; Zhao,
> Wei <wei.zhao@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2] net/ice: remove unnecessary variable
> 
> Hi, Qi
> 
> On 03/03, Qi Zhang wrote:
> >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.
> >
> >In a validate opertion, a meta is not necessary be created during
> >parser, so NULL will be parsed to low level engine and all engine's
> >parse_pattern_action need to be modified properly.
> >
> >The the patch also fix a potentional memory leak in
> 
> s/fix/fixes
> 
> >ice_parse_engine_validate, since meta may not be freed.
> >
> >Fixes: 4e27d3ed02bd ("net/ice: fix flow API framework")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >---
> >
> >v2:
> >- remove meta free to low level engine
> >- allow parse meta = NULL to parse_pattern_action.
> >
> > drivers/net/ice/ice_fdir_filter.c   |  3 ++-
> > drivers/net/ice/ice_generic_flow.c  | 24 ++++++++++--------------
> > drivers/net/ice/ice_hash.c          | 13 +++++++------
> > drivers/net/ice/ice_switch_filter.c | 14 +++++++-------
> > 4 files changed, 26 insertions(+), 28 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..6bcf28de8 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,7
> >+1725,7 @@ 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; @@ -1737,7 +1736,7 @@
> >ice_parse_engine_create(struct ice_adapter *ad,
> > 			continue;
> > 		}
> >
> >-		ret = engine->create(ad, flow, *meta, error);
> >+		ret = engine->create(ad, flow, meta, error);
> > 		if (ret == 0)
> > 			return engine;
> > 		else if (ret == -EEXIST)
> >@@ -1752,7 +1751,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 +1761,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;
> > 	}
> >+
> 
> Irrelevant change.
> 
> > 	return engine;
> > }
> >
> >@@ -1779,7 +1778,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 +1812,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 +1820,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 +1843,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 +1860,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 +1870,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..b37da9920 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);
> >
> >@@ -503,12 +506,10 @@ ice_hash_create(struct ice_adapter *ad,
> >
> > out:
> > 	flow->rule = filter_ptr;
> >-	rte_free(meta);
> > 	return 0;
> >
> > error:
> > 	rte_free(filter_ptr);
> >-	rte_free(meta);
> > 	return -rte_errno;
> > }
> >
> >diff --git a/drivers/net/ice/ice_switch_filter.c
> >b/drivers/net/ice/ice_switch_filter.c
> >index 4a9356b31..8a2ba8aa6 100644
> >--- a/drivers/net/ice/ice_switch_filter.c
> >+++ b/drivers/net/ice/ice_switch_filter.c
> >@@ -249,13 +249,10 @@ ice_switch_create(struct ice_adapter *ad,
> > 	}
> >
> > 	rte_free(list);
> >-	rte_free(meta);
> > 	return 0;
> >
> > error:
> > 	rte_free(list);
> >-	rte_free(meta);
> >-
> > 	return -rte_errno;
> > }
> >
> >@@ -1088,10 +1085,13 @@ 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;
> >+	}
> 
> If meta is NULL for ice_parse_engine_validate case, do we need free
> sw_meta_ptr explicitly in switch like we free(rss_meta_ptr) in hash? And also
> free the list?

Yes, we need.

> 
> 
> And for case meta is not NULL, who is responsible for freeing it eventually?

Seems I forgot to rollback all the meta data free in create ops.

Thanks
Qi

> 
> Thanks,
> Xiaolong
> 
> > 	rte_free(pattern_match_item);
> >
> > 	return 0;
> >--
> >2.13.6
> >