From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tiwei Bie Subject: Re: [PATCH v2 02/17] net/i40e: store tunnel filter Date: Wed, 28 Dec 2016 11:27:27 +0800 Message-ID: <20161228032726.GA8024@dpdk19> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-3-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 6954B2C4F for ; Wed, 28 Dec 2016 04:32:51 +0100 (CET) Content-Disposition: inline In-Reply-To: <1482819984-14120-3-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:09PM +0800, Beilei Xing wrote: > Currently there's no tunnel filter stored in SW. > This patch stores tunnel filter in SW with cuckoo > hash, also adds protection if a tunnel filter has > been added. > > Signed-off-by: Beilei Xing > --- > drivers/net/i40e/i40e_ethdev.c | 167 ++++++++++++++++++++++++++++++++++++++++- > drivers/net/i40e/i40e_ethdev.h | 27 +++++++ > 2 files changed, 191 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index 80dd8d7..c012d5d 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c [...] > @@ -1267,6 +1314,7 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev) > hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > pci_dev = dev->pci_dev; > ethertype_rule = &pf->ethertype; > + tunnel_rule = &pf->tunnel; > > if (hw->adapter_stopped == 0) > i40e_dev_close(dev); > @@ -1283,6 +1331,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev) > rte_free(p_ethertype); > } > > + /* Remove all tunnel director rules and hash */ > + if (tunnel_rule->hash_map) > + rte_free(tunnel_rule->hash_map); > + if (tunnel_rule->hash_table) > + rte_hash_free(tunnel_rule->hash_table); > + > + while ((p_tunnel = TAILQ_FIRST(&tunnel_rule->tunnel_list))) { There is a redundant pair of parentheses, or you should compare with NULL. > + TAILQ_REMOVE(&tunnel_rule->tunnel_list, p_tunnel, rules); > + rte_free(p_tunnel); > + } > + > dev->dev_ops = NULL; > dev->rx_pkt_burst = NULL; > dev->tx_pkt_burst = NULL; > @@ -6482,6 +6541,81 @@ i40e_dev_get_filter_type(uint16_t filter_type, uint16_t *flag) > return 0; > } > > +/* Convert tunnel filter structure */ > +static int > +i40e_tunnel_filter_convert(struct i40e_aqc_add_remove_cloud_filters_element_data > + *cld_filter, > + struct i40e_tunnel_filter *tunnel_filter) > +{ > + ether_addr_copy((struct ether_addr *)&cld_filter->outer_mac, > + (struct ether_addr *)&tunnel_filter->input.outer_mac); > + ether_addr_copy((struct ether_addr *)&cld_filter->inner_mac, > + (struct ether_addr *)&tunnel_filter->input.inner_mac); > + tunnel_filter->input.inner_vlan = cld_filter->inner_vlan; > + tunnel_filter->input.flags = cld_filter->flags; > + tunnel_filter->input.tenant_id = cld_filter->tenant_id; > + tunnel_filter->queue = cld_filter->queue_number; > + > + return 0; > +} > + > +/* Check if there exists the tunnel filter */ > +static struct i40e_tunnel_filter * > +i40e_sw_tunnel_filter_lookup(struct i40e_tunnel_rule *tunnel_rule, > + const struct i40e_tunnel_filter_input *input) > +{ > + int ret = 0; > + The initialization is meaningless, as it will be written by the below assignment unconditionally. > + ret = rte_hash_lookup(tunnel_rule->hash_table, (const void *)input); > + if (ret < 0) > + return NULL; > + > + return tunnel_rule->hash_map[ret]; > +} > + > +/* Add a tunnel filter into the SW list */ > +static int > +i40e_sw_tunnel_filter_insert(struct i40e_pf *pf, > + struct i40e_tunnel_filter *tunnel_filter) > +{ > + struct i40e_tunnel_rule *tunnel_rule = &pf->tunnel; > + int ret = 0; > + Same here. > + ret = rte_hash_add_key(tunnel_rule->hash_table, > + &tunnel_filter->input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to insert tunnel filter to hash table %d!", > + ret); Function should return when ret < 0. > + tunnel_rule->hash_map[ret] = tunnel_filter; > + > + TAILQ_INSERT_TAIL(&tunnel_rule->tunnel_list, tunnel_filter, rules); > + > + return 0; > +} > + > +/* Delete a tunnel filter from the SW list */ > +static int > +i40e_sw_tunnel_filter_del(struct i40e_pf *pf, > + struct i40e_tunnel_filter *tunnel_filter) > +{ > + struct i40e_tunnel_rule *tunnel_rule = &pf->tunnel; > + int ret = 0; > + Same here. > + ret = rte_hash_del_key(tunnel_rule->hash_table, > + &tunnel_filter->input); > + if (ret < 0) > + PMD_DRV_LOG(ERR, > + "Failed to delete tunnel filter to hash table %d!", > + ret); Function should return when ret < 0. > + tunnel_rule->hash_map[ret] = NULL; > + > + TAILQ_REMOVE(&tunnel_rule->tunnel_list, tunnel_filter, rules); > + rte_free(tunnel_filter); > + > + return 0; > +} > + > static int > i40e_dev_tunnel_filter_set(struct i40e_pf *pf, > struct rte_eth_tunnel_filter_conf *tunnel_filter, > @@ -6497,6 +6631,8 @@ i40e_dev_tunnel_filter_set(struct i40e_pf *pf, > struct i40e_vsi *vsi = pf->main_vsi; > struct i40e_aqc_add_remove_cloud_filters_element_data *cld_filter; > struct i40e_aqc_add_remove_cloud_filters_element_data *pfilter; > + struct i40e_tunnel_rule *tunnel_rule = &pf->tunnel; > + struct i40e_tunnel_filter *tunnel, *node; > > cld_filter = rte_zmalloc("tunnel_filter", > sizeof(struct i40e_aqc_add_remove_cloud_filters_element_data), > @@ -6559,11 +6695,36 @@ i40e_dev_tunnel_filter_set(struct i40e_pf *pf, > pfilter->tenant_id = rte_cpu_to_le_32(tunnel_filter->tenant_id); > pfilter->queue_number = rte_cpu_to_le_16(tunnel_filter->queue_id); > > - if (add) > + tunnel = rte_zmalloc("tunnel_filter", sizeof(*tunnel), 0); > + i40e_tunnel_filter_convert(cld_filter, tunnel); > + node = i40e_sw_tunnel_filter_lookup(tunnel_rule, &tunnel->input); > + if (add && node) { > + PMD_DRV_LOG(ERR, "Conflict with existing tunnel rules!"); > + rte_free(tunnel); > + 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