From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xing, Beilei" Subject: Re: [PATCH 10/24] ethdev: parse ethertype filter Date: Wed, 21 Dec 2016 03:54:50 +0000 Message-ID: <94479800C636CB44BD422CB454846E013157F36E@SHSMSX101.ccr.corp.intel.com> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1480679625-4157-11-git-send-email-beilei.xing@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Lu, Wenzhuo" , Adrien Mazarguil To: "Yigit, Ferruh" , "Wu, Jingjing" , "Zhang, Helin" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id EDE5FFC09 for ; Wed, 21 Dec 2016 04:54:54 +0100 (CET) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Ferruh, > -----Original Message----- > From: Yigit, Ferruh > Sent: Wednesday, December 21, 2016 2:12 AM > To: Xing, Beilei ; Wu, Jingjing > ; Zhang, Helin > Cc: dev@dpdk.org; Lu, Wenzhuo ; Adrien Mazarguil > > Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter >=20 > 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 > > Signed-off-by: Beilei Xing > > --- >=20 > CC: Adrien Mazarguil >=20 > > lib/librte_ether/rte_flow.c | 136 > +++++++++++++++++++++++++++++++++++++ > > lib/librte_ether/rte_flow_driver.h | 34 ++++++++++ >=20 > <...> >=20 > > 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); >=20 > Although this is helper function, it may be good if it follows the rte_fo= llow > namespace. OK, I will rename it in the next version, thanks very much. >=20 > > + > > +#define PATTERN_SKIP_VOID(filter, filter_struct, error_type) > \ > > + do { \ > > + if (!pattern) { \ > > + memset(filter, 0, sizeof(filter_struct)); \ > > + error->type =3D error_type; \ > > + return -EINVAL; > \ > > + } \ > > + item =3D pattern + i; \ >=20 > 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. >=20 > > + while (item->type =3D=3D RTE_FLOW_ITEM_TYPE_VOID) { > \ > > + i++; \ > > + item =3D pattern + i; \ > > + } \ > > + } while (0) > > + > > +#define ACTION_SKIP_VOID(filter, filter_struct, error_type) > \ > > + do { \ > > + if (!actions) { \ > > + memset(filter, 0, sizeof(filter_struct)); \ > > + error->type =3D error_type; \ > > + return -EINVAL; > \ > > + } \ > > + act =3D actions + i; \ > > + while (act->type =3D=3D RTE_FLOW_ACTION_TYPE_VOID) { \ > > + i++; \ > > + act =3D actions + i; \ > > + } \ > > + } while (0) >=20 > Are these macros generic enough for all rte_flow consumers? >=20 > 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 ther= e'll be many such codes to get the next non-void item in all parse function= s, including the parse_ethertype_filter function in rte_flow.c. But actuall= y I'm not very sure if it's generic enough for all consumers, although I th= ink it's general at present:)=20 Thanks for your advice, I'll move the macros to PMD currently, then there'l= l be no macros used in parse_ethertype_filter function, and optimize it aft= er applied. BTW, I plan to send out V2 patch set in this week. Best Regards, Beilei >=20 > > + > > #ifdef __cplusplus > > } > > #endif > >