From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tiwei Bie Subject: Re: [PATCH v2 03/17] net/i40e: store flow director filter Date: Wed, 28 Dec 2016 11:38:48 +0800 Message-ID: <20161228033847.GB13841@dpdk19> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-4-git-send-email-beilei.xing@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: jingjing.wu@intel.com, helin.zhang@intel.com, dev@dpdk.org To: Beilei Xing Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 5211A282 for ; Wed, 28 Dec 2016 04:44:12 +0100 (CET) Content-Disposition: inline In-Reply-To: <1482819984-14120-4-git-send-email-beilei.xing@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Dec 27, 2016 at 02:26:10PM +0800, Beilei Xing wrote: > Currently there's no flow director filter stored in SW. This > patch stores flow director filters in SW with cuckoo hash, > also adds protection if a flow director filter has been added. > > Signed-off-by: Beilei Xing > --- > drivers/net/i40e/i40e_ethdev.c | 48 +++++++++++++++++++++ > drivers/net/i40e/i40e_ethdev.h | 12 ++++++ > drivers/net/i40e/i40e_fdir.c | 98 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 158 insertions(+) > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index c012d5d..427ebdc 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c [...] > @@ -1342,6 +1379,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev) > rte_free(p_tunnel); > } > > + /* Remove all flow director rules and hash */ > + if (fdir_info->hash_map) > + rte_free(fdir_info->hash_map); > + if (fdir_info->hash_table) > + rte_hash_free(fdir_info->hash_table); > + > + while ((p_fdir = TAILQ_FIRST(&fdir_info->fdir_list))) { There is a redundant pair of parentheses, or you should compare with NULL. > + TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules); > + rte_free(p_fdir); > + } > + > dev->dev_ops = NULL; > dev->rx_pkt_burst = NULL; > dev->tx_pkt_burst = NULL; [...] > diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c > index 335bf15..faa2495 100644 > --- a/drivers/net/i40e/i40e_fdir.c > +++ b/drivers/net/i40e/i40e_fdir.c [...] > +/* Check if there exists the flow director filter */ > +static struct i40e_fdir_filter * > +i40e_sw_fdir_filter_lookup(struct i40e_fdir_info *fdir_info, > + const struct rte_eth_fdir_input *input) > +{ > + int ret = 0; > + The initialization is meaningless, as it will be written by the below assignment unconditionally. > + ret = rte_hash_lookup(fdir_info->hash_table, (const void *)input); > + if (ret < 0) > + return NULL; > + > + return fdir_info->hash_map[ret]; > +} > + > +/* Add a flow director filter into the SW list */ > +static int > +i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter *filter) > +{ > + struct i40e_fdir_info *fdir_info = &pf->fdir; > + int ret = 0; > + Same here. > + ret = rte_hash_add_key(fdir_info->hash_table, > + &filter->fdir.input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to insert fdir filter to hash table %d!", > + ret); Function should return when ret < 0. > + fdir_info->hash_map[ret] = filter; > + > + TAILQ_INSERT_TAIL(&fdir_info->fdir_list, filter, rules); > + > + return 0; > +} > + > +/* Delete a flow director filter from the SW list */ > +static int > +i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter *filter) > +{ > + struct i40e_fdir_info *fdir_info = &pf->fdir; > + int ret = 0; > + Same here. > + ret = rte_hash_del_key(fdir_info->hash_table, > + &filter->fdir.input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to delete fdir filter to hash table %d!", > + ret); Function should return when ret < 0. > + fdir_info->hash_map[ret] = NULL; > + > + TAILQ_REMOVE(&fdir_info->fdir_list, filter, rules); > + rte_free(filter); > + > + return 0; > +} > + > /* > * i40e_add_del_fdir_filter - add or remove a flow director filter. > * @pf: board private structure > @@ -1032,6 +1105,8 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev, > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > unsigned char *pkt = (unsigned char *)pf->fdir.prg_pkt; > enum i40e_filter_pctype pctype; > + struct i40e_fdir_info *fdir_info = &pf->fdir; > + struct i40e_fdir_filter *fdir_filter, *node; > int ret = 0; > > if (dev->data->dev_conf.fdir_conf.mode != RTE_FDIR_MODE_PERFECT) { > @@ -1054,6 +1129,21 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev, > return -EINVAL; > } > > + fdir_filter = rte_zmalloc("fdir_filter", sizeof(*fdir_filter), 0); > + i40e_fdir_filter_convert(filter, fdir_filter); > + node = i40e_sw_fdir_filter_lookup(fdir_info, &fdir_filter->fdir.input); > + if (add && node) { > + PMD_DRV_LOG(ERR, > + "Conflict with existing flow director rules!"); > + rte_free(fdir_filter); > + return -EINVAL; > + } else if (!add && !node) { When `if (add && node)' is true, function will return. There is no need to use `else' here. Best regards, Tiwei Bie