net/ice: remove unnecessary variable
diff mbox series

Message ID 20200225131418.5750-1-qi.z.zhang@intel.com
State Changes Requested
Delegated to: xiaolong ye
Headers show
Series
  • net/ice: remove unnecessary variable
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/checkpatch success coding style OK

Commit Message

Zhang, Qi Z Feb. 25, 2020, 1:14 p.m. UTC
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.

The patch also move the meta's memory free from each filter
engine->create to upper layer, the memory leakage when create
a fdir rule is also be fixed.

Fixes: f5cafa961fae ("net/ice: add flow director create and destroy")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/ice/ice_generic_flow.c  | 28 ++++++++++++++--------------
 drivers/net/ice/ice_hash.c          |  2 --
 drivers/net/ice/ice_switch_filter.c |  3 ---
 3 files changed, 14 insertions(+), 19 deletions(-)

Comments

Ye Xiaolong March 2, 2020, 8:19 a.m. UTC | #1
Hi, Qi

On 02/25, 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.
>
>The patch also move the meta's memory free from each filter

s/move/moves

>engine->create to upper layer, the memory leakage when create
>a fdir rule is also be fixed.
>
>Fixes: f5cafa961fae ("net/ice: add flow director create and destroy")
>Cc: stable@dpdk.org
>
>Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>---
> drivers/net/ice/ice_generic_flow.c  | 28 ++++++++++++++--------------
> drivers/net/ice/ice_hash.c          |  2 --
> drivers/net/ice/ice_switch_filter.c |  3 ---
> 3 files changed, 14 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/net/ice/ice_generic_flow.c b/drivers/net/ice/ice_generic_flow.c
>index 38ac799d8..f22538758 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,9 @@ ice_parse_engine_create(struct ice_adapter *ad,
> 			continue;
> 		}
> 
>-		ret = engine->create(ad, flow, *meta, error);
>+		ret = engine->create(ad, flow, meta, error);
>+		if (meta)
>+			rte_free(meta);

Maybe I miss something, I see meta in fdir's parse_pattern_action is assigned as 
&pf->fdir.conf which is not dynamically allocated like sw_meta_ptr in switch and 
rss_meta_ptr in hash, is it valid to call rte_free(meta) for fdir?
And theoretically, if engine->create is NULL, is there a memory leak for meta?

Thanks,
Xiaolong
Zhang, Qi Z March 2, 2020, 11:56 p.m. UTC | #2
> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Monday, March 2, 2020 4:20 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; Cao, Yahui <yahui.cao@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ice: remove unnecessary variable
> 
> Hi, Qi
> 
> On 02/25, 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.
> >
> >The patch also move the meta's memory free from each filter
> 
> s/move/moves
> 
> >engine->create to upper layer, the memory leakage when create
> >a fdir rule is also be fixed.
> >
> >Fixes: f5cafa961fae ("net/ice: add flow director create and destroy")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >---
> > drivers/net/ice/ice_generic_flow.c  | 28 ++++++++++++++--------------
> > drivers/net/ice/ice_hash.c          |  2 --
> > drivers/net/ice/ice_switch_filter.c |  3 ---
> > 3 files changed, 14 insertions(+), 19 deletions(-)
> >
> >diff --git a/drivers/net/ice/ice_generic_flow.c
> >b/drivers/net/ice/ice_generic_flow.c
> >index 38ac799d8..f22538758 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,9 @@
> >ice_parse_engine_create(struct ice_adapter *ad,
> > 			continue;
> > 		}
> >
> >-		ret = engine->create(ad, flow, *meta, error);
> >+		ret = engine->create(ad, flow, meta, error);
> >+		if (meta)
> >+			rte_free(meta);
> 
> Maybe I miss something, I see meta in fdir's parse_pattern_action is assigned as
> &pf->fdir.conf which is not dynamically allocated like sw_meta_ptr in switch
> and rss_meta_ptr in hash, is it valid to call rte_free(meta) for fdir?
> And theoretically, if engine->create is NULL, is there a memory leak for meta?

Good point, meta release function could be implemented in low level engine and but should be directed by up layer, so I think the missing part is a new op like engine->meta_release.

Thanks
Qi
> 
> Thanks,
> Xiaolong

Patch
diff mbox series

diff --git a/drivers/net/ice/ice_generic_flow.c b/drivers/net/ice/ice_generic_flow.c
index 38ac799d8..f22538758 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,9 @@  ice_parse_engine_create(struct ice_adapter *ad,
 			continue;
 		}
 
-		ret = engine->create(ad, flow, *meta, error);
+		ret = engine->create(ad, flow, meta, error);
+		if (meta)
+			rte_free(meta);
 		if (ret == 0)
 			return engine;
 		else if (ret == -EEXIST)
@@ -1752,23 +1753,25 @@  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;
 	struct ice_flow_parser_node *parser_node;
+	void *meta = NULL;
 	void *temp;
 
 	TAILQ_FOREACH_SAFE(parser_node, parser_list, node, temp) {
 		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;
 		break;
 	}
+	if (meta)
+		rte_free(meta);
 	return engine;
 }
 
@@ -1779,7 +1782,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 +1816,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 +1824,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 +1847,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 +1864,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 +1874,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..fc3f33ee1 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -503,12 +503,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..314a9a096 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;
 }