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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

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