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

["Xing, Beilei" <beilei.xing@intel.com>]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, December 28, 2016 10:22 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Helin
> <helin.zhang@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