Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter

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

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, December 23, 2016 4:43 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>;
> dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter
> 
> Hi all,
> 
> On Wed, Dec 21, 2016 at 03:54:50AM +0000, Xing, Beilei wrote:
> > Hi Ferruh,
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh
> > > Sent: Wednesday, December 21, 2016 2:12 AM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> > > Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> > > Mazarguil <adrien.mazarguil@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter
> > >
> > > On 12/2/2016 11:53 AM, Beilei Xing wrote:
> > > > Check if the rule is a ethertype rule, and get the ethertype info BTW.
> > > >
> > > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > > ---
> > >
> > > CC: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Thanks again for CC'ing me.
> 
> > > >  lib/librte_ether/rte_flow.c        | 136
> > > +++++++++++++++++++++++++++++++++++++
> > > >  lib/librte_ether/rte_flow_driver.h |  34 ++++++++++
> > >
> > > <...>
> > >
> > > > diff --git a/lib/librte_ether/rte_flow_driver.h
> > > > b/lib/librte_ether/rte_flow_driver.h
> > > > index a88c621..2760c74 100644
> > > > --- a/lib/librte_ether/rte_flow_driver.h
> > > > +++ b/lib/librte_ether/rte_flow_driver.h
> > > > @@ -170,6 +170,40 @@ rte_flow_error_set(struct rte_flow_error
> > > > *error, const struct rte_flow_ops *  rte_flow_ops_get(uint8_t
> > > > port_id, struct rte_flow_error *error);
> > > >
> > > > +int cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
> > > > +			    const struct rte_flow_item *pattern,
> > > > +			    const struct rte_flow_action *actions,
> > > > +			    struct rte_eth_ethertype_filter *filter,
> > > > +			    struct rte_flow_error *error);
> > >
> > > Although this is helper function, it may be good if it follows the
> > > rte_follow namespace.
> >
> > OK, I will rename it in the next version, thanks very much.
> 
> Agreed, all public symbols exposed by headers must be prefixed with
> rte_flow.
> 
> Now I'm not so sure about the need to convert a rte_flow rule to a
> rte_eth_ethertype_filter. This definition basically makes rte_flow depend on
> rte_eth_ctrl.h (related #include is missing by the way).
> 

Since the whole implementation of parse function is modified, there'll be no common rte_eth_ethertype_filter here temporarily.

> I understand that both ixgbe and i40e would benefit from it, and considering
> rte_flow_driver.h is free from ABI versioning I guess it's acceptable, but
> remember we'll gradually remove existing filter types so we should avoid
> new dependencies on them. Just keep in mind this will be temporary.
> 

 i40e and ixgbe all use existing filter types in rte_flow_driver.h. if all existing filter types will be removed, we need to change the fiter info after applied.

> Please add full documentation as well in Doxygen style like for existing
> symbols. We have to maintain this API properly documented.
> 
> > > > +
> > > > +#define PATTERN_SKIP_VOID(filter, filter_struct, error_type)
> > > 	\
> > > > +	do {								\
> > > > +		if (!pattern) {						\
> > > > +			memset(filter, 0, sizeof(filter_struct));	\
> > > > +			error->type = error_type;                       \
> > > > +			return -EINVAL;
> > > 	\
> > > > +		}							\
> > > > +		item = pattern + i;					\
> > >
> > > I believe macros that relies on variables that not passed as
> > > argument is not good idea.
> >
> > Yes, I'm reworking the macros, and it will be changed in v2.
> >
> > >
> > > > +		while (item->type == RTE_FLOW_ITEM_TYPE_VOID) {
> > > 	\
> > > > +			i++;						\
> > > > +			item = pattern + i;				\
> > > > +		}							\
> > > > +	} while (0)
> > > > +
> > > > +#define ACTION_SKIP_VOID(filter, filter_struct, error_type)
> > > 	\
> > > > +	do {								\
> > > > +		if (!actions) {						\
> > > > +			memset(filter, 0, sizeof(filter_struct));	\
> > > > +			error->type = error_type;			\
> > > > +			return -EINVAL;
> > > 	\
> > > > +		}							\
> > > > +		act = actions + i;					\
> > > > +		while (act->type == RTE_FLOW_ACTION_TYPE_VOID) {	\
> > > > +			i++;						\
> > > > +			act = actions + i;				\
> > > > +		}							\
> > > > +	} while (0)
> > >
> > > Are these macros generic enough for all rte_flow consumers?
> > >
> > > What do you think separate this patch, and use these after applied,
> > > meanwhile keeping function and MACROS PMD internal?
> >
> > The main purpose of the macros is to reduce the code in PMD, otherwise
> > there'll be many such codes to get the next non-void item in all parse
> > functions, including the parse_ethertype_filter function in
> > rte_flow.c. But actually I'm not very sure if it's generic enough for
> > all consumers, although I think it's general at present:)
> 
> I'll concede skipping VOIDs can be tedious depending on the parser
> implementation, but I do not think these macros need to be exposed either.
> PMDs can duplicate some code such as this.
> 
> I think ixgbe and i40e share a fair amount of code already, and factoring it
> should be part of larger task to create a common Intel-specific library instead.


Good point. Thanks. We'll consider related implementation for the common code.
In V2 patch set, there'll be no common code temporarily since the implementation of parsing functions is different between ixgbe and i40e.

> 
> > Thanks for your advice, I'll move the macros to PMD currently, then there'll
> be no macros used in parse_ethertype_filter function, and optimize it after
> applied.
> >
> > BTW, I plan to send out V2 patch set in this week.
> >
> > Best Regards,
> > Beilei
> >
> > >
> > > > +
> > > >  #ifdef __cplusplus
> > > >  }
> > > >  #endif
> > > >
> >
> 
> --
> Adrien Mazarguil
> 6WIND