From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xing, Beilei" Subject: Re: [PATCH 10/24] ethdev: parse ethertype filter Date: Tue, 27 Dec 2016 06:36:58 +0000 Message-ID: <94479800C636CB44BD422CB454846E013158B8D8@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> <94479800C636CB44BD422CB454846E013157F36E@SHSMSX101.ccr.corp.intel.com> <20161223084328.GI10340@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Yigit, Ferruh" , "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" , "Lu, Wenzhuo" To: Adrien Mazarguil Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id BC0585588 for ; Tue, 27 Dec 2016 07:37:18 +0100 (CET) In-Reply-To: <20161223084328.GI10340@6wind.com> 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" > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, December 23, 2016 4:43 PM > To: Xing, Beilei > Cc: Yigit, Ferruh ; Wu, Jingjing > ; Zhang, Helin ; > dev@dpdk.org; Lu, Wenzhuo > Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter >=20 > Hi all, >=20 > 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 ; Wu, Jingjing > > > ; Zhang, Helin > > > Cc: dev@dpdk.org; Lu, Wenzhuo ; Adrien > > > Mazarguil > > > 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 B= TW. > > > > > > > > Signed-off-by: Wenzhuo Lu > > > > Signed-off-by: Beilei Xing > > > > --- > > > > > > CC: Adrien Mazarguil >=20 > Thanks again for CC'ing me. >=20 > > > > 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. >=20 > Agreed, all public symbols exposed by headers must be prefixed with > rte_flow. >=20 > 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). >=20 Since the whole implementation of parse function is modified, there'll be n= o common rte_eth_ethertype_filter here temporarily. > I understand that both ixgbe and i40e would benefit from it, and consider= ing > rte_flow_driver.h is free from ABI versioning I guess it's acceptable, bu= t > remember we'll gradually remove existing filter types so we should avoid > new dependencies on them. Just keep in mind this will be temporary. >=20 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 aft= er applied. > Please add full documentation as well in Doxygen style like for existing > symbols. We have to maintain this API properly documented. >=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; \ > > > > > > 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 =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) > > > > > > 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:) >=20 > 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. >=20 > 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 i= nstead. Good point. Thanks. We'll consider related implementation for the common co= de. In V2 patch set, there'll be no common code temporarily since the implement= ation of parsing functions is different between ixgbe and i40e. >=20 > > Thanks for your advice, I'll move the macros to PMD currently, then the= re'll > 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 > > > > > > > > > + > > > > #ifdef __cplusplus > > > > } > > > > #endif > > > > > > >=20 > -- > Adrien Mazarguil > 6WIND