From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id EF5D410E06 for ; Fri, 23 Dec 2016 09:43:36 +0100 (CET) Received: by mail-wm0-f47.google.com with SMTP id t79so206899957wmt.0 for ; Fri, 23 Dec 2016 00:43:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=3UU7sJ8GNSPWdYPjrj00Hxd0l4J3H/s8WgN0SVT9AEU=; b=a7zluuiYsh23e9NYSd6Qo6UIBMGMDNIkOEddzibwGPh2wc7ZXGih0a/xZ2aThW1VuR HHa9l66YBznzbCr/YM46aShqJ45A4ZCsIv+r6AafIkI2LD/sKthvGX0UzTuOMeYKlyPy Xso2bcPK/NQAGy7GYdT76Mb9aHR9b831lQWpd3Oo6CSEP18Osc/jogPJ1ZzCjw8unRFc 2GfQuY1DBHdnDUvcn8dKyaxEmREYfm0P6hjtkyWP0ZrKRCiP7RVYH1I+sUbbKocIlByX UoI8dQhQDV0slE25RT3HCnhA7fxpzTfnhDhjD0ogSmX7Axf57HVb7x3PQb3hV6hNOE0z I0Jw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=3UU7sJ8GNSPWdYPjrj00Hxd0l4J3H/s8WgN0SVT9AEU=; b=Xc0VrVB9AenP/d9yW/B5T6ZRbR/gcJ2aBaj9yQtNLKwKms+pBjXJ+ODKvtCoEjyk8B tDNwiEGnabMgoejQjMCcVf8DGEt4HQxv7t7qO0d47zCMeYINo15o3O+LBRIJIDtYQ4uH rkN4ywjDqW4C0V071b+jhj7OqI7b6iE7k8+d7yYZktguUkiWqDMZfjfbl9m3VAwnIPjU ttUvKCEFsZOhL1fTUarANdxVRk4s7t5c2adUXwPKyOf3adYxYu9T0VkUFefJ0T4j4nOD y0rW818LGxvYVeDuDJ2nFIot/nTrr4f7r8AkyuzIX3LGhfFvUFIOLLVFMJ5qCAQqirnx cakg== X-Gm-Message-State: AIkVDXKqqLuzAVKYXJHTvZgncF6Fi/mUxx8LW/ch3IJa+j5+KzC0qDSjdaBnOPEc9GwYUPzh X-Received: by 10.28.69.17 with SMTP id s17mr13089206wma.141.1482482616594; Fri, 23 Dec 2016 00:43:36 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id ia7sm39317928wjb.23.2016.12.23.00.43.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Dec 2016 00:43:35 -0800 (PST) Date: Fri, 23 Dec 2016 09:43:28 +0100 From: Adrien Mazarguil To: "Xing, Beilei" Cc: "Yigit, Ferruh" , "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" , "Lu, Wenzhuo" Message-ID: <20161223084328.GI10340@6wind.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <94479800C636CB44BD422CB454846E013157F36E@SHSMSX101.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Dec 2016 08:43:37 -0000 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 ; 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 BTW. > > > > > > Signed-off-by: Wenzhuo Lu > > > Signed-off-by: Beilei Xing > > > --- > > > > CC: Adrien Mazarguil 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