From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 3D12F3777 for ; Wed, 28 Dec 2016 06:41:22 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP; 27 Dec 2016 21:41:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,420,1477983600"; d="scan'208";a="1105101502" Received: from dpdk19.sh.intel.com (HELO dpdk19) ([10.239.129.113]) by fmsmga002.fm.intel.com with ESMTP; 27 Dec 2016 21:41:19 -0800 Date: Wed, 28 Dec 2016 13:35:56 +0800 From: Tiwei Bie To: Beilei Xing Cc: jingjing.wu@intel.com, helin.zhang@intel.com, dev@dpdk.org Message-ID: <20161228053556.GB28245@dpdk19> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-16-git-send-email-beilei.xing@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1482819984-14120-16-git-send-email-beilei.xing@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [dpdk-dev] [PATCH v2 15/17] net/i40e: add flow flush function 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: , X-List-Received-Date: Wed, 28 Dec 2016 05:41:22 -0000 On Tue, Dec 27, 2016 at 02:26:22PM +0800, Beilei Xing wrote: > This patch adds i40e_flow_flush function to flush all > filters for users. And flow director flush function > is involved first. > > Signed-off-by: Beilei Xing > --- > drivers/net/i40e/i40e_ethdev.h | 3 +++ > drivers/net/i40e/i40e_fdir.c | 8 ++------ > drivers/net/i40e/i40e_flow.c | 46 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h > index b8c7d41..0b736d5 100644 > --- a/drivers/net/i40e/i40e_ethdev.h > +++ b/drivers/net/i40e/i40e_ethdev.h > @@ -786,6 +786,9 @@ i40e_sw_tunnel_filter_lookup(struct i40e_tunnel_rule *tunnel_rule, > const struct i40e_tunnel_filter_input *input); > int i40e_sw_tunnel_filter_del(struct i40e_pf *pf, > struct i40e_tunnel_filter *tunnel_filter); > +int i40e_sw_fdir_filter_del(struct i40e_pf *pf, > + struct i40e_fdir_filter *filter); > +int i40e_fdir_flush(struct rte_eth_dev *dev); > Why don't declare them as the global functions at the beginning? > /* I40E_DEV_PRIVATE_TO */ > #define I40E_DEV_PRIVATE_TO_PF(adapter) \ > diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c > index 6c1bb18..f10aeee 100644 > --- a/drivers/net/i40e/i40e_fdir.c > +++ b/drivers/net/i40e/i40e_fdir.c > @@ -119,8 +119,6 @@ static int i40e_fdir_filter_programming(struct i40e_pf *pf, > enum i40e_filter_pctype pctype, > const struct rte_eth_fdir_filter *filter, > bool add); > -static int i40e_fdir_flush(struct rte_eth_dev *dev); > - > static int i40e_fdir_filter_convert(const struct rte_eth_fdir_filter *input, > struct i40e_fdir_filter *filter); > static struct i40e_fdir_filter * > @@ -128,8 +126,6 @@ i40e_sw_fdir_filter_lookup(struct i40e_fdir_info *fdir_info, > const struct rte_eth_fdir_input *input); > static int i40e_sw_fdir_filter_insert(struct i40e_pf *pf, > struct i40e_fdir_filter *filter); > -static int i40e_sw_fdir_filter_del(struct i40e_pf *pf, > - struct i40e_fdir_filter *filter); > > static int > i40e_fdir_rx_queue_init(struct i40e_rx_queue *rxq) > @@ -1070,7 +1066,7 @@ i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter *filter) > } > > /* Delete a flow director filter from the SW list */ > -static int > +int > i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter *filter) > { > struct i40e_fdir_info *fdir_info = &pf->fdir; > @@ -1318,7 +1314,7 @@ i40e_fdir_filter_programming(struct i40e_pf *pf, > * i40e_fdir_flush - clear all filters of Flow Director table > * @pf: board private structure > */ > -static int > +int > i40e_fdir_flush(struct rte_eth_dev *dev) > { > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c > index 4c7856c..1d9f603 100644 > --- a/drivers/net/i40e/i40e_flow.c > +++ b/drivers/net/i40e/i40e_flow.c > @@ -68,6 +68,8 @@ static struct rte_flow *i40e_flow_create(struct rte_eth_dev *dev, > const struct rte_flow_item pattern[], > const struct rte_flow_action actions[], > struct rte_flow_error *error); > +static int i40e_flow_flush(struct rte_eth_dev *dev, > + struct rte_flow_error *error); > static int i40e_flow_destroy(struct rte_eth_dev *dev, > struct rte_flow *flow, > struct rte_flow_error *error); > @@ -95,11 +97,13 @@ static int i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf, > struct i40e_ethertype_filter *filter); > static int i40e_dev_destroy_tunnel_filter(struct i40e_pf *pf, > struct i40e_tunnel_filter *filter); > +static int i40e_fdir_filter_flush(struct i40e_pf *pf); > > const struct rte_flow_ops i40e_flow_ops = { > .validate = i40e_flow_validate, > .create = i40e_flow_create, > .destroy = i40e_flow_destroy, > + .flush = i40e_flow_flush, > }; > > enum rte_filter_type cons_filter_type = RTE_ETH_FILTER_NONE; > @@ -1603,3 +1607,45 @@ i40e_dev_destroy_tunnel_filter(struct i40e_pf *pf, > > return ret; > } > + > +static int > +i40e_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error) > +{ > + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > + int ret = 0; > + Meaningless initialization. > + ret = i40e_fdir_filter_flush(pf); > + if (!ret) > + rte_flow_error_set(error, EINVAL, > + RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > + "Failed to flush FDIR flows."); Just curious. What's the relationship between `ret' and the code (EINVAL) passed to rte_flow_error_set()? Is `-ret' acceptable as the parameter? Because the `-ret' which is actually returned by i40e_fdir_flush() is also some standard UNIX errno. When error occurs, user should use which one to figure out the failure reason? `-ret' or `rte_errno'? > + > + return ret; > +} > + > +static int > +i40e_fdir_filter_flush(struct i40e_pf *pf) > +{ > + struct rte_eth_dev *dev = pf->adapter->eth_dev; > + struct i40e_fdir_info *fdir_info = &pf->fdir; > + struct i40e_fdir_filter *fdir_filter; > + struct i40e_flow *flow; > + int ret = 0; > + Meaningless initialization. > + ret = i40e_fdir_flush(dev); > + if (!ret) { > + /* Delete FDIR filters in FDIR list. */ > + while ((fdir_filter = TAILQ_FIRST(&fdir_info->fdir_list))) > + i40e_sw_fdir_filter_del(pf, fdir_filter); > + The i40e_sw_fdir_filter_del() may fail, in which case fdir_filter won't be removed from fdir_info->fdir_list. Will it lead to an infinite loop? Should you check the retval of i40e_sw_fdir_filter_del() and break the loop when it fails? Best regards, Tiwei Bie