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

[Xing, Beilei]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 2:12 AM
> To: Xing, Beilei <beilei.xing at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; Zhang, Helin <helin.zhang at intel.com>
> Cc: dev at dpdk.org; Lu, Wenzhuo <wenzhuo.lu at intel.com>; Adrien Mazarguil
> <adrien.mazarguil at 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 at intel.com>
> > Signed-off-by: Beilei Xing <beilei.xing at intel.com>
> > ---
> 
> CC: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> 
> >  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.

> 
> > +
> > +#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:) 
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
> >