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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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).

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.

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.

> 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