From: Ye Xiaolong <xiaolong.ye@intel.com>
To: Qi Zhang <qi.z.zhang@intel.com>
Cc: yahui.cao@intel.com, simei.su@intel.com, wei.zhao@intel.com,
dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable
Date: Tue, 3 Mar 2020 16:27:11 +0800 [thread overview]
Message-ID: <20200303082711.GI25927@intel.com> (raw)
In-Reply-To: <20200303041800.44155-1-qi.z.zhang@intel.com>
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
>
next prev parent reply other threads:[~2020-03-03 8:29 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-03 4:18 [dpdk-dev] [PATCH v2] net/ice: remove unnecessary variable Qi Zhang
2020-03-03 8:27 ` Ye Xiaolong [this message]
2020-03-03 8:35 ` Zhang, Qi Z
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200303082711.GI25927@intel.com \
--to=xiaolong.ye@intel.com \
--cc=dev@dpdk.org \
--cc=qi.z.zhang@intel.com \
--cc=simei.su@intel.com \
--cc=stable@dpdk.org \
--cc=wei.zhao@intel.com \
--cc=yahui.cao@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.