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

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

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 <beilei.xing@intel.com>
> ---
>  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(&ethertype_rule->ethertype_list))) {

There is a redundant pair of parentheses, or you should compare with
NULL.

> +		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;
> @@ -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(&ethertype_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(&ethertype_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,
> +					       &ethertype_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