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 013DCA056B; Tue, 3 Mar 2020 09:35:51 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6718D1C00D; Tue, 3 Mar 2020 09:35:51 +0100 (CET) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 27A341BFF9; Tue, 3 Mar 2020 09:35:48 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Mar 2020 00:35:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,510,1574150400"; d="scan'208";a="274117423" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 03 Mar 2020 00:35:48 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Mar 2020 00:35:47 -0800 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Mar 2020 00:35:47 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.137]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.86]) with mapi id 14.03.0439.000; Tue, 3 Mar 2020 16:35:45 +0800 From: "Zhang, Qi Z" To: "Ye, Xiaolong" CC: "Cao, Yahui" , "Su, Simei" , "Zhao, Wei" , "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v2] net/ice: remove unnecessary variable Thread-Index: AQHV8RJBXuB+o8xQnUOjOJ8ILLdSyKg2AtGAgACHp4A= Date: Tue, 3 Mar 2020 08:35:45 +0000 Message-ID: <039ED4275CED7440929022BC67E70611547D9455@SHSMSX103.ccr.corp.intel.com> References: <20200303041800.44155-1-qi.z.zhang@intel.com> <20200303082711.GI25927@intel.com> In-Reply-To: <20200303082711.GI25927@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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" > -----Original Message----- > From: Ye, Xiaolong > Sent: Tuesday, March 3, 2020 4:27 PM > To: Zhang, Qi Z > Cc: Cao, Yahui ; Su, Simei ; Zha= o, > Wei ; dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH v2] net/ice: remove unnecessary variable >=20 > Hi, Qi >=20 > 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 >=20 > s/fix/fixes >=20 > >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 =3D 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 =3D filter; > >+ if (meta) > >+ *meta =3D 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 =3D NULL; > > struct ice_flow_parser_node *parser_node; > >+ void *meta =3D 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 =3D parser_node->parser->engine; @@ -1737,7 +1736,7 @@ > >ice_parse_engine_create(struct ice_adapter *ad, > > continue; > > } > > > >- ret =3D engine->create(ad, flow, *meta, error); > >+ ret =3D engine->create(ad, flow, meta, error); > > if (ret =3D=3D 0) > > return engine; > > else if (ret =3D=3D -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 =3D 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 =3D parser_node->parser->engine; > > break; > > } > >+ >=20 > Irrelevant change. >=20 > > 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 =3D ice_parse_engine(ad, flow, &pf->rss_parser_list, > >- pattern, actions, meta, error); > >+ pattern, actions, error); > > if (*engine !=3D 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 =3D ice_parse_engine(ad, flow, &pf->dist_parser_list, > >- pattern, actions, meta, error); > >+ pattern, actions, error); > > break; > > case ICE_FLOW_CLASSIFY_STAGE_PERMISSION: > > *engine =3D 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 =3D NULL; > > int ret; > > struct ice_flow_engine *engine =3D NULL; > >- void *meta; > > > > flow =3D rte_zmalloc("ice_flow", sizeof(struct rte_flow), 0); > > if (!flow) { > >@@ -1874,7 +1870,7 @@ ice_flow_create(struct rte_eth_dev *dev, > > } > > > > ret =3D 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 =3D 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 =3D rss_meta_ptr; > >- ((struct rss_meta *)*meta)->pkt_hdr =3D ((struct rss_type_match_hdr *) > >+ rss_meta_ptr->pkt_hdr =3D ((struct rss_type_match_hdr *) > > (pattern_match_item->meta))->hdr_mask; > > > > /* Check rss action. */ > >- ret =3D ice_hash_parse_action(pattern_match_item, actions, meta, error= ); > >+ ret =3D ice_hash_parse_action(pattern_match_item, actions, > >+ (void **)&rss_meta_ptr, error); > >+ > > error: > >- if (ret) > >+ if (!ret && meta) > >+ *meta =3D 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 =3D 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 =3D sw_meta_ptr; > >- ((struct sw_meta *)*meta)->list =3D list; > >- ((struct sw_meta *)*meta)->lkups_num =3D lkups_num; > >- ((struct sw_meta *)*meta)->rule_info =3D rule_info; > >+ > >+ if (meta) { > >+ *meta =3D sw_meta_ptr; > >+ ((struct sw_meta *)*meta)->list =3D list; > >+ ((struct sw_meta *)*meta)->lkups_num =3D lkups_num; > >+ ((struct sw_meta *)*meta)->rule_info =3D rule_info; > >+ } >=20 > 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. >=20 >=20 > And for case meta is not NULL, who is responsible for freeing it eventual= ly? Seems I forgot to rollback all the meta data free in create ops. Thanks Qi >=20 > Thanks, > Xiaolong >=20 > > rte_free(pattern_match_item); > > > > return 0; > >-- > >2.13.6 > >