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

[Ferruh Yigit]

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.x...@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.

<...>