Re: [dpdk-dev] [PATCH v6 01/17] net/i40e: store ethertype filter

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/5/2017 3:45 PM, 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 <beilei.xing@intel.com>
> ---

<...>

> @@ -939,9 +946,18 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
>  	int ret;
>  	uint32_t len;
>  	uint8_t aq_fail = 0;
> +	struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype;
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> +	char ethertype_hash_name[RTE_HASH_NAMESIZE];
> +	struct rte_hash_parameters ethertype_hash_params = {
> +		.name = ethertype_hash_name,
> +		.entries = I40E_MAX_ETHERTYPE_FILTER_NUM,
> +		.key_len = sizeof(struct i40e_ethertype_filter_input),
> +		.hash_func = rte_hash_crc,
> +	};
> +
>  	dev->dev_ops = &i40e_eth_dev_ops;
>  	dev->rx_pkt_burst = i40e_recv_pkts;
>  	dev->tx_pkt_burst = i40e_xmit_pkts;
> @@ -1182,8 +1198,33 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
>  		pf->flags &= ~I40E_FLAG_DCB;
>  	}
>  
> +	/* Initialize ethertype filter rule list and hash */
> +	TAILQ_INIT(&ethertype_rule->ethertype_list);
> +	snprintf(ethertype_hash_name, RTE_HASH_NAMESIZE,
> +		 "ethertype_%s", dev->data->name);
> +	ethertype_rule->hash_table = rte_hash_create(&ethertype_hash_params);
> +	if (!ethertype_rule->hash_table) {
> +		PMD_INIT_LOG(ERR, "Failed to create ethertype hash table!");
> +		ret = -EINVAL;
> +		goto err_ethertype_hash_table_create;
> +	}
> +	ethertype_rule->hash_map = rte_zmalloc("i40e_ethertype_hash_map",
> +				       sizeof(struct i40e_ethertype_filter *) *
> +				       I40E_MAX_ETHERTYPE_FILTER_NUM,
> +				       0);
> +	if (!ethertype_rule->hash_map) {
> +		PMD_INIT_LOG(ERR,
> +		     "Failed to allocate memory for ethertype hash map!");
> +		ret = -ENOMEM;
> +		goto err_ethertype_hash_map_alloc;
> +	}
> +
>  	return 0;
>  
> +err_ethertype_hash_map_alloc:
> +	rte_hash_free(ethertype_rule->hash_table);
> +err_ethertype_hash_table_create:
> +	rte_free(dev->data->mac_addrs);
>  err_mac_alloc:
>  	i40e_vsi_release(pf->main_vsi);
>  err_setup_pf_switch:

It can be good idea to extract filter related code into a separate
function, eth_i40e_dev_init() is already too big. It is up to you.


> @@ -1206,25 +1247,42 @@ 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 rte_intr_handle *intr_handle;
>  	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 = I40E_DEV_TO_PCI(dev);
>  	intr_handle = &pci_dev->intr_handle;
> +	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(&ethertype_rule->ethertype_list))) {
> +		TAILQ_REMOVE(&ethertype_rule->ethertype_list,
> +			     p_ethertype, rules);
> +		rte_free(p_ethertype);
> +	}
> +
>  	dev->dev_ops = NULL;
>  	dev->rx_pkt_burst = NULL;
>  	dev->tx_pkt_burst = NULL;

Same is valid for  eth_i40e_dev_uninit(), if possible having a separate
function for filter related work.

<...>