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

[Xing, Beilei]

> -----Original Message-----
> From: Xing, Beilei
> Sent: Thursday, December 29, 2016 12:03 PM
> To: Wu, Jingjing <jingjing...@intel.com>; Zhang, Helin
> <helin.zh...@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 01/17] net/i40e: store ethertype filter
> 
> > -----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.

Sorry, you are right. In fact I needn't to malloc before convert. Will update 
it in next version.

> 
> >
> > Thanks
> > Jingjing