From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f174.google.com (mail-wj0-f174.google.com [209.85.210.174]) by dpdk.org (Postfix) with ESMTP id 807AE10DDF for ; Fri, 23 Dec 2016 09:13:19 +0100 (CET) Received: by mail-wj0-f174.google.com with SMTP id sd9so61441017wjb.1 for ; Fri, 23 Dec 2016 00:13:19 -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=PhEAx0CBoRKJVsSmQ3tVRUFxygSntYYlPgEA0DfEA6Q=; b=wO9jOnYSVbyU4lbrNuQi2e4sZ00Yw1Rv9JjU50z909Kp6M3Jr/9PLCjvRdaa+jpO3u 1PTlukldxXVUxgov+d45RnUhSpBYeZFQ4ijOUIuddGAcsJNf60qQA/xf7nNv9ome1XPG sNV89g52arzTw9TtWOrqv5m0lRySF75yzwXK4BbSdp1Gh9BtpcU6JOtDEX3h9R0Rr+xJ DNhjIdUpEXFqpq8UoFcwzin+mbhc9nWBzBzyJ6X0CF8PRV+lapOMJx9diUOgPYnEEfDS Sh76pXnL7KFRqkyDUv/dRx3lS05Nfmh5sH/JURb6toXD6XL+37CR1SoRxMXeYA/ICP7C IBzQ== 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=PhEAx0CBoRKJVsSmQ3tVRUFxygSntYYlPgEA0DfEA6Q=; b=mtY6APLXkukARZDPDqSBpJ7Yfwq0KiV5aVOWRYxpANeYYEB7iCmWbIwXjmGMAAW1i+ 46LWRsojO7YfrUvaTfagkNiKuNpIKgWiK4sTTsuLDM/8SlVWswYiT2sHA6hp+FHwFHSa li5boBXrRAZgABigy9AyqNe2P/sbeLCbbXEWhKozdbMR65MTfuJmtHi9XfomGXgn2pOs rQLtantn2tU2gALOewvl8qscoHcdiUUYYHkndPWryz2apDnIaCmrDQPG0xUfFKI1kT7W GaLIjpzbYepmxlKp/EV/kujkkly/wwbkbWUGqFQwJdBSPnW8JxOMbLa8iE6lfBo/If00 /g7g== X-Gm-Message-State: AIkVDXL9JKkyjbw3sG3fpskzj043u9aEMSxdRF/KPoUtlUgm0Dk93dzHqBnizGuZL3TFVae7 X-Received: by 10.194.134.66 with SMTP id pi2mr11883791wjb.61.1482480799231; Fri, 23 Dec 2016 00:13:19 -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 63sm35887930wmg.2.2016.12.23.00.13.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Dec 2016 00:13:18 -0800 (PST) Date: Fri, 23 Dec 2016 09:13:10 +0100 From: Adrien Mazarguil To: Ferruh Yigit Cc: "Zhao1, Wei" , "dev@dpdk.org" , "Lu, Wenzhuo" Message-ID: <20161223081310.GH10340@6wind.com> References: <1480675394-59179-1-git-send-email-wei.zhao1@intel.com> <1480675394-59179-16-git-send-email-wei.zhao1@intel.com> <86cc3f7c-4488-9efa-48ce-82eb57ae572c@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86cc3f7c-4488-9efa-48ce-82eb57ae572c@intel.com> Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director 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:13:19 -0000 Hi, On Thu, Dec 22, 2016 at 10:44:32AM +0000, Ferruh Yigit wrote: > On 12/22/2016 9:19 AM, Zhao1, Wei wrote: > > Hi, Yigit > > > >> -----Original Message----- > >> From: Yigit, Ferruh > >> Sent: Wednesday, December 21, 2016 1:01 AM > >> To: Zhao1, Wei ; dev@dpdk.org > >> Cc: Lu, Wenzhuo > >> Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter > >> > >> On 12/2/2016 10:43 AM, Wei Zhao wrote: > >>> From: wei zhao1 > >>> > >>> check if the rule is a flow director rule, and get the flow director info. > >>> > >>> Signed-off-by: wei zhao1 > >>> Signed-off-by: Wenzhuo Lu > >>> --- > >> > >> <...> > >> > >>> + PATTERN_SKIP_VOID(rule, struct ixgbe_fdir_rule, > >>> + RTE_FLOW_ERROR_TYPE_ITEM_NUM); > >>> + if (item->type != RTE_FLOW_ITEM_TYPE_ETH && > >>> + item->type != RTE_FLOW_ITEM_TYPE_IPV4 && > >>> + item->type != RTE_FLOW_ITEM_TYPE_IPV6 && > >>> + item->type != RTE_FLOW_ITEM_TYPE_UDP && > >>> + item->type != RTE_FLOW_ITEM_TYPE_VXLAN && > >>> + item->type != RTE_FLOW_ITEM_TYPE_NVGRE) { > >> > >> This gives build error [1], there are a few more same usage: > >> > >> .../drivers/net/ixgbe/ixgbe_ethdev.c:9238:17: error: comparison of constant > >> 241 with expression of type 'const enum rte_flow_item_type' is always true > >> [-Werror,-Wtautological-constant-out-of-range-compare] > >> item->type != RTE_FLOW_ITEM_TYPE_NVGRE) { > >> > >> > >> > > > > Ok, I will add two type definition RTE_FLOW_ITEM_TYPE_NVGRE and RTE_FLOW_ITEM_TYPE_E_TAG into const enum rte_flow_item_type to eliminate this problem. > > Thank you. > > > > CC: Adrien Mazarguil Thanks, the original message did not catch my attention. > Yes, that is what right thing to do, since rte_flow patchset not merged > yet, perhaps Adrien may want to include this as next version of his > patchset? > > What do you think Adrien? I think by now the rte_flow series is automatically categorized as spam by half the community already, and that new items such as these can be added subsequently on their own, ideally before the entire ixgbe/i40e series. I have a few comments regarding these new items if we want to make them part of rte_flow.h (see definitions below). Unfortunately, even though they are super convenient to use and expose in the testpmd flow command, we need to avoid C-style bit-field definitions such as these for protocol header matching because they are not endian-safe, particularly multi-byte fields. Only meta-data that should be interpreted with host endianness can use them (e.g. struct rte_flow_attr, struct rte_flow_action_vf, etc): struct rte_flow_item_nvgre { uint32_t flags0:1; /**< 0 */ uint32_t rsvd1:1; /**< 1 bit not defined */ uint32_t flags1:2; /**< 2 bits, 1 0 */ uint32_t rsvd0:9; /**< Reserved0 */ uint32_t ver:3; /**< version */ uint32_t protocol:16; /**< protocol type, 0x6558 */ uint32_t tni:24; /**< tenant network ID or virtual subnet ID */ uint32_t flow_id:8; /**< flow ID or Reserved */ }; For an example how to avoid them, see struct ipv6_hdr definition in rte_ip.h, where field vtc_flow is 32 bit but covers three protocol fields and is considered big-endian (Nelio's endianness series [1] would be really handy to eliminate confusion here). Also see struct rte_flow_item_vxlan, which covers 24-bit fields using uint8_t arrays. struct rte_flow_item_e_tag { struct ether_addr dst; /**< Destination MAC. */ struct ether_addr src; /**< Source MAC. */ uint16_t e_tag_ethertype; /**< E-tag EtherType, 0x893F. */ uint16_t e_pcp:3; /**< E-PCP */ uint16_t dei:1; /**< DEI */ uint16_t in_e_cid_base:12; /**< Ingress E-CID base */ uint16_t rsv:2; /**< reserved */ uint16_t grp:2; /**< GRP */ uint16_t e_cid_base:12; /**< E-CID base */ uint16_t in_e_cid_ext:8; /**< Ingress E-CID extend */ uint16_t e_cid_ext:8; /**< E-CID extend */ uint16_t type; /**< MAC type. */ unsigned int tags; /**< Number of 802.1Q/ad tags defined. */ struct { uint16_t tpid; /**< Tag protocol identifier. */ uint16_t tci; /**< Tag control information. */ } tag[]; /**< 802.1Q/ad tag definitions, outermost first. */ }; Besides the bit-field issue for this one, looks like it should be split, at least the initial part is already covered by rte_flow_item_eth. I do not know much about E-Tag (IEEE 802.1BR right?) but it sort of looks like a 2.5 layer protocol reminiscent of VLAN. tags and tag[] fields seem based on the VLAN definition of the original rte_flow RFC that has since been replaced with stacked rte_flow_item_vlan items, much easier to program for. Perhaps this can be relied on instead of having e_tag implement its own variant. As a protocol-matching item and E-Tag TCI being 6 bytes according to IEEE 802.1BR, sizeof(struct rte_flow_item_e_tag) should ideally return 6 as well and would likely comprise three big-endian uint16_t fields. See how PCP/DEI/VID VLAN fields are interpreted in testpmd [2]. Again, these concerns only stand if you intend to include these definitions into rte_flow.h. [1] http://dpdk.org/ml/archives/dev/2016-November/050060.html [2] http://dpdk.org/ml/archives/dev/2016-December/052976.html -- Adrien Mazarguil 6WIND