From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tiwei Bie Subject: Re: [PATCH v2 01/17] net/i40e: store ethertype filter Date: Wed, 28 Dec 2016 11:22:39 +0800 Message-ID: <20161228032238.GA13841@dpdk19> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-2-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 mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 134872C4F for ; Wed, 28 Dec 2016 04:28:03 +0100 (CET) Content-Disposition: inline In-Reply-To: <1482819984-14120-2-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:08PM +0800, Beilei Xing wrote: > Currently there's no ethertype filter stored in SW. > This patch stores ethertype filter with cuckoo hash > in SW, also adds protection if an ethertype filter > has been added. > > Signed-off-by: Beilei Xing > --- > drivers/net/i40e/Makefile | 1 + > drivers/net/i40e/i40e_ethdev.c | 164 ++++++++++++++++++++++++++++++++++++++++- > drivers/net/i40e/i40e_ethdev.h | 26 +++++++ > 3 files changed, 190 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/i40e/Makefile b/drivers/net/i40e/Makefile > index 66997b6..11175c4 100644 > --- a/drivers/net/i40e/Makefile > +++ b/drivers/net/i40e/Makefile > @@ -117,5 +117,6 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += lib/librte_eal lib/librte_ether > DEPDIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += lib/librte_mempool lib/librte_mbuf > DEPDIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += lib/librte_net > DEPDIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += lib/librte_kvargs > +DEPDIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += lib/librte_hash > > include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index f42f4ba..80dd8d7 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c [...] > @@ -1203,23 +1249,40 @@ eth_i40e_dev_init(struct rte_eth_dev *dev) > static int > eth_i40e_dev_uninit(struct rte_eth_dev *dev) > { > + struct i40e_pf *pf; > struct rte_pci_device *pci_dev; > struct i40e_hw *hw; > struct i40e_filter_control_settings settings; > + struct i40e_ethertype_filter *p_ethertype; > int ret; > uint8_t aq_fail = 0; > + struct i40e_ethertype_rule *ethertype_rule; > > PMD_INIT_FUNC_TRACE(); > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > return 0; > > + pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > pci_dev = dev->pci_dev; > + ethertype_rule = &pf->ethertype; > > if (hw->adapter_stopped == 0) > i40e_dev_close(dev); > > + /* Remove all ethertype director rules and hash */ > + if (ethertype_rule->hash_map) > + rte_free(ethertype_rule->hash_map); > + if (ethertype_rule->hash_table) > + rte_hash_free(ethertype_rule->hash_table); > + > + while ((p_ethertype = TAILQ_FIRST(ðertype_rule->ethertype_list))) { There is a redundant pair of parentheses, or you should compare with NULL. > + TAILQ_REMOVE(ðertype_rule->ethertype_list, > + p_ethertype, rules); > + rte_free(p_ethertype); > + } > + > dev->dev_ops = NULL; > dev->rx_pkt_burst = NULL; > dev->tx_pkt_burst = NULL; > @@ -7954,6 +8017,78 @@ i40e_hash_filter_ctrl(struct rte_eth_dev *dev, > return ret; > } > > +/* Convert ethertype filter structure */ > +static int > +i40e_ethertype_filter_convert(const struct rte_eth_ethertype_filter *input, > + struct i40e_ethertype_filter *filter) > +{ > + rte_memcpy(&filter->input.mac_addr, &input->mac_addr, ETHER_ADDR_LEN); > + filter->input.ether_type = input->ether_type; > + filter->flags = input->flags; > + filter->queue = input->queue; > + > + return 0; > +} > + > +/* Check if there exists the ehtertype filter */ > +static struct i40e_ethertype_filter * > +i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_rule *ethertype_rule, > + const struct i40e_ethertype_filter_input *input) > +{ > + int ret = 0; > + The initialization is meaningless, as it will be written by the below assignment unconditionally. > + ret = rte_hash_lookup(ethertype_rule->hash_table, (const void *)input); > + if (ret < 0) > + return NULL; > + > + return ethertype_rule->hash_map[ret]; > +} > + > +/* Add ethertype filter in SW list */ > +static int > +i40e_sw_ethertype_filter_insert(struct i40e_pf *pf, > + struct i40e_ethertype_filter *filter) > +{ > + struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype; > + int ret = 0; > + Same here. > + ret = rte_hash_add_key(ethertype_rule->hash_table, > + &filter->input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to insert ethertype filter" > + " to hash table %d!", > + ret); Function should return when ret < 0. > + ethertype_rule->hash_map[ret] = filter; > + > + TAILQ_INSERT_TAIL(ðertype_rule->ethertype_list, filter, rules); > + > + return 0; > +} > + > +/* Delete ethertype filter in SW list */ > +static int > +i40e_sw_ethertype_filter_del(struct i40e_pf *pf, > + struct i40e_ethertype_filter *filter) > +{ > + struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype; > + int ret = 0; > + Same here. > + ret = rte_hash_del_key(ethertype_rule->hash_table, > + &filter->input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to delete ethertype filter" > + " to hash table %d!", > + ret); Function should return when ret < 0. > + ethertype_rule->hash_map[ret] = NULL; > + > + TAILQ_REMOVE(ðertype_rule->ethertype_list, filter, rules); > + rte_free(filter); > + > + return 0; > +} > + > /* > * Configure ethertype filter, which can director packet by filtering > * with mac address and ether_type or only ether_type > @@ -7964,6 +8099,8 @@ i40e_ethertype_filter_set(struct i40e_pf *pf, > bool add) > { > struct i40e_hw *hw = I40E_PF_TO_HW(pf); > + struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype; > + struct i40e_ethertype_filter *ethertype_filter, *node; > struct i40e_control_filter_stats stats; > uint16_t flags = 0; > int ret; > @@ -7982,6 +8119,22 @@ i40e_ethertype_filter_set(struct i40e_pf *pf, > PMD_DRV_LOG(WARNING, "filter vlan ether_type in first tag is" > " not supported."); > > + /* Check if there is the filter in SW list */ > + ethertype_filter = rte_zmalloc("ethertype_filter", > + sizeof(*ethertype_filter), 0); > + i40e_ethertype_filter_convert(filter, ethertype_filter); > + node = i40e_sw_ethertype_filter_lookup(ethertype_rule, > + ðertype_filter->input); > + if (add && node) { > + PMD_DRV_LOG(ERR, "Conflict with existing ethertype rules!"); > + rte_free(ethertype_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