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

[Xing, Beilei]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, December 28, 2016 10:22 AM
> To: Xing, Beilei <beilei.x...@intel.com>; Zhang, Helin
> <helin.zh...@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 01/17] net/i40e: store ethertype filter
> 
> > +
> > +/* 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;
> > +
> > +   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);
> > +   ethertype_rule->hash_map[ret] = NULL;
> > +
> > +   TAILQ_REMOVE(&ethertype_rule->ethertype_list, filter, rules);
> > +   rte_free(filter);
> 
> It's better to free filter out of del function because the filter is also the 
> input
> argument.
> Or you can define this function to use key as argument but not filter.
Thanks for the suggestion, I'll use the key as parameter in the next version.

> 
> >  /*
> >   * 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) {
> > +           PMD_DRV_LOG(ERR, "There's no corresponding ethertype
> > filter!");
> > +           rte_free(ethertype_filter);
> > +           return -EINVAL;
> > +   }
> How about malloc ethertype_filter after check? Especially, no need to malloc
> it when delete a filter.

Malloc ethertype_filter because i40e_ethertype_filter_convert is involved 
first, and then check if a rule exists with ethertype_filter->input.

> 
> Thanks
> Jingjing