Re: [dpdk-dev] [PATCH v2 02/17] net/i40e: store tunnel filter

[Tiwei Bie <tiwei.bie@intel.com>]

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 <beilei.xing@intel.com>
> ---
>  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