From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B8097A056B; Tue, 3 Mar 2020 09:29:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7438B1BFF2; Tue, 3 Mar 2020 09:29:28 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4459D1BFF0; Tue, 3 Mar 2020 09:29:26 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Mar 2020 00:29:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,510,1574150400"; d="scan'208";a="438632181" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by fmsmga005.fm.intel.com with ESMTP; 03 Mar 2020 00:29:22 -0800 Date: Tue, 3 Mar 2020 16:27:11 +0800 From: Ye Xiaolong To: Qi Zhang Cc: yahui.cao@intel.com, simei.su@intel.com, wei.zhao@intel.com, dev@dpdk.org, stable@dpdk.org Message-ID: <20200303082711.GI25927@intel.com> References: <20200303041800.44155-1-qi.z.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200303041800.44155-1-qi.z.zhang@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >--- > >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 >