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

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

> -----Original Message-----
> From: Xing, Beilei
> Sent: Thursday, December 29, 2016 12:03 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin
> <helin.zhang@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.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.

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

> 
> >
> > Thanks
> > Jingjing