From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 59A0A2B91 for ; Tue, 27 Dec 2016 04:31:33 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP; 26 Dec 2016 19:31:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,414,1477983600"; d="scan'208";a="43532395" Received: from kmsmsx153.gar.corp.intel.com ([172.21.73.88]) by orsmga004.jf.intel.com with ESMTP; 26 Dec 2016 19:31:30 -0800 Received: from pgsmsx103.gar.corp.intel.com ([169.254.2.52]) by KMSMSX153.gar.corp.intel.com ([172.21.73.88]) with mapi id 14.03.0248.002; Tue, 27 Dec 2016 11:31:29 +0800 From: "Zhao1, Wei" To: Adrien Mazarguil , "Yigit, Ferruh" CC: "dev@dpdk.org" , "Lu, Wenzhuo" Thread-Topic: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter Thread-Index: AQHSTIl1UqOHvKDzk0WcYUuowCyJo6EQpf0AgAMpDAD//5KJAIABaAoAgAaAXMA= Date: Tue, 27 Dec 2016 03:31:28 +0000 Message-ID: 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> <20161223081310.GH10340@6wind.com> In-Reply-To: <20161223081310.GH10340@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Tue, 27 Dec 2016 03:31:33 -0000 Hi, Adrien > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, December 23, 2016 4:13 PM > To: Yigit, Ferruh > Cc: Zhao1, Wei ; dev@dpdk.org; Lu, Wenzhuo > > Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filt= er >=20 > Hi, >=20 > 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 directo= r 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 !=3D RTE_FLOW_ITEM_TYPE_ETH && > > >>> + item->type !=3D RTE_FLOW_ITEM_TYPE_IPV4 && > > >>> + item->type !=3D RTE_FLOW_ITEM_TYPE_IPV6 && > > >>> + item->type !=3D RTE_FLOW_ITEM_TYPE_UDP && > > >>> + item->type !=3D RTE_FLOW_ITEM_TYPE_VXLAN && > > >>> + item->type !=3D 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 !=3D 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 >=20 > Thanks, the original message did not catch my attention. >=20 > > 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? >=20 > I think by now the rte_flow series is automatically categorized as spam b= y > 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. >=20 > I have a few comments regarding these new items if we want to make them > part of rte_flow.h (see definitions below). >=20 oK , I will add these type definition by my patch in v2 > 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 inter= preted > with host endianness can use them (e.g. struct rte_flow_attr, struct > rte_flow_action_vf, etc): >=20 > 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 */ }; >=20 > For an example how to avoid them, see struct ipv6_hdr definition in rte_i= p.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 hand= y to > eliminate confusion here). Also see struct rte_flow_item_vxlan, which > covers 24-bit fields using uint8_t arrays. >=20 > 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. */ }; >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 we= ll > and would likely comprise three big-endian uint16_t fields. See how > PCP/DEI/VID VLAN fields are interpreted in testpmd [2]. >=20 > Again, these concerns only stand if you intend to include these definitio= ns > into rte_flow.h. >=20 > [1] http://dpdk.org/ml/archives/dev/2016-November/050060.html > [2] http://dpdk.org/ml/archives/dev/2016-December/052976.html >=20 > -- > Adrien Mazarguil > 6WIND