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 66E6B1B795 for ; Thu, 12 Apr 2018 07:13:56 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2018 22:13:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,440,1517904000"; d="scan'208";a="50132206" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 11 Apr 2018 22:13:53 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 11 Apr 2018 22:13:53 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 11 Apr 2018 22:13:53 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.151]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.197]) with mapi id 14.03.0319.002; Thu, 12 Apr 2018 13:12:09 +0800 From: "Zhang, Qi Z" To: Adrien Mazarguil CC: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" Thread-Topic: [PATCH v2 3/4] ether: add more protocol support in flow API Thread-Index: AQHTyjsqd8vtfSJ73EmHUBFJggYaiaP7SreAgAFA3jA= Date: Thu, 12 Apr 2018 05:12:08 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153190216@SHSMSX103.ccr.corp.intel.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-4-git-send-email-qi.z.zhang@intel.com> <20180411163213.GN4957@6wind.com> In-Reply-To: <20180411163213.GN4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 3/4] ether: add more protocol support in flow API 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: Thu, 12 Apr 2018 05:13:57 -0000 Hi Adrien: Thank you so much for your careful review and helpful suggestions! I agree with most of your comments, except couple question about RTE_FLOW_= ITEM_TYPE_TGT_ADDR and RTE_FLOW_ITEM_IPV6_EXT_HDR Please see my comment inline. Thanks! Qi > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Thursday, April 12, 2018 12:32 AM > To: Zhang, Qi Z > Cc: dev@dpdk.org; Doherty, Declan ; Chandran, > Sugesh ; Glynn, Michael J > ; Liu, Yu Y ; Ananyev, > Konstantin ; Richardson, Bruce > > Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API >=20 > On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote: > > Add new protocol header match support as below > > > > RTE_FLOW_ITEM_TYPE_ARP > > - match IPv4 ARP header > > RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY > > - match any IPv6 extension header >=20 > While properly defined in the patch, "IPV6" is missing here. >=20 > > RTE_FLOW_ITEM_TYPE_ICMPV6 > > - match IPv6 ICMP header > > RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR > > - match IPv6 ICMP Target address > > RTE_FLOW_ITEM_TYPE_ICMPV6_SSL > > - match IPv6 ICMP Source Link-layer address > > RTE_FLOW_ITEM_TYPE_ICMPV6_TTL > > - match IPv6 ICMP Target Link-layer address > > > > Signed-off-by: Qi Zhang >=20 > First, since they are added at the end of enum rte_flow_item_type, no ABI > breakage notice is necessary. >=20 > However testpmd implementation [1][2] and documentation update [3][4] are > mandatory for all new pattern items and actions. OK, will add this into v2. >=20 > More comments below regarding these definitions. >=20 > [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an exampl= e > in app/test-pmd/cmdline_flow.c [3] "Pattern items" section in > doc/guides/testpmd_app_ug/testpmd_funcs.rst > [4] using "Item: ``ICMP``" section as an example in > doc/guides/prog_guide/rte_flow.rst >=20 > > --- > > lib/librte_ether/rte_flow.h | 160 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 160 insertions(+) > > > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > > index 8f75db0..a8ec780 100644 > > --- a/lib/librte_ether/rte_flow.h > > +++ b/lib/librte_ether/rte_flow.h > > @@ -323,6 +323,49 @@ enum rte_flow_item_type { > > * See struct rte_flow_item_geneve. > > */ > > RTE_FLOW_ITEM_TYPE_GENEVE, > > + > > + /** > > + * Matches ARP IPv4 header. >=20 > =3D> Matches an IPv4 ARP header. >=20 > > + * > > + * See struct rte_flow_item_arp. > > + */ > > + RTE_FLOW_ITEM_TYPE_ARP, >=20 > While you're right to make "IPv4" clear since ARP is also used for other > protocols DPDK doesn't support (and likely never will), the ARP header ha= s > both a fixed and a variably-sized part. >=20 > Ideally an ARP pattern item should match the fixed part only and a separa= te > ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP below. >=20 > Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so one > suggestion would be to rename this pattern item ARP_IPV4 directly: >=20 > =3D> RTE_FLOW_ITEM_TYPE_ARP_IPV4 >=20 > > + > > + /** > > + * Matches any IPv6 Extension header. >=20 > =3D> Matches an IPv6 extension header. >=20 > > + * > > + * See struct rte_flow_item_ipv6_ext_any. > > + */ > > + RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY, >=20 > I'm not sure this definition is necessary, more below about that. >=20 > Also I don't see a benefit in having "ANY" part of the name, if you want = to keep > it, I suggest the simpler: >=20 > =3D> RTE_FLOW_ITEM_TYPE_IPV6_EXT >=20 > > + > > + /** > > + * Matches ICMPv6 header. >=20 > =3D> Matches an ICMPv6 header. >=20 > > + * > > + * See struct rte_flow_item_icmpv6 >=20 > Missing "." >=20 > > + */ > > + RTE_FLOW_ITEM_TYPE_ICMPV6, > > + >=20 > Before entering NDP territory below, I understand those should be stacked= on > top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for clarity they should b= e > named after the NDP types they represent, not inner data fields. >=20 > Also I think we should consider NDP as a protocol sitting on top of ICMPv= 6. We > could therefore drop "ICMP" from these definitions. >=20 > Since "ND" is a common shorthand for this protocol and "6" another when > doing something related to IPv6, I suggest to use "ND6" to name he relate= d > pattern items. I agree. >=20 > These are the reasons behind my next suggestions: >=20 > > + /** > > + * Match ICMPv6 target address. > > + * > > + * See struct rte_flow_item_icmpv6_tgt_addr. > > + */ > > + RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR, >=20 > =3D> Matches an IPv6 network discovery router solicitation. > =3D> See struct rte_flow_item_nd6_rs. > =3D> RTE_FLOW_ITEM_TYPE_ND6_RS, >=20 > You should add another item for neighbor advertisement messages using the > same template: >=20 > =3D> Match an IPv6 network discovery neighbor advertisement. > =3D> See struct rte_flow_item_nd6_na. > =3D> RTE_FLOW_ITEM_TYPE_ND6_NA, The purpose of RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR is to match a "target add= ress" according to IPv6 ND spec https://tools.ietf.org/html/rfc4861#page-22, when= type =3D 135/136 so do you mean we should have RTE_FLOW_ITEM_TYPE_ND6_NS (Neighbor Solicitat= ion) and RTE_FLOW_ITEM_TYPE_ND6_NA (Neighbor Advertisement) here, and with the same template (an IPV6 addr) for rte_flow_item_icmpv6_tgt_addr= ? >=20 > The following are possible options for these headers, if specified they m= ust be > found afterward. Also since IPv6 may run on top of protocols other than > Ethernet, you need to clarify these link-layer addresses use the Ethernet > format: >=20 > > + > > + /** > > + * Match ICMPv6 Source Link-Layer Address. > > + * > > + * See struct rte_flow_item_icmpv6_sll. > > + */ > > + RTE_FLOW_ITEM_TYPE_ICMPV6_SLL, >=20 > =3D> Matches an IPv6 network discovery source Ethernet link-layer address > option. > =3D> See struct rte_flow_item_nd6_opt_sla_eth. > =3D> RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH, >=20 > > + > > + /** > > + * Match ICMPv6 Target Link-Layer Address. > > + * > > + * See struct rte_flow_item_icmpv6_tll. > > + */ > > + RTE_FLOW_ITEM_TYPE_ICMPV6_TLL, >=20 > =3D> Matches an IPv6 network discovery target Ethernet link-layer address > option. > =3D> See struct rte_flow_item_nd6_opt_tla_eth. > =3D> RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH, >=20 Agree to rename. > > + >=20 > Unnecessary empty line. >=20 > > }; > > > > /** > > @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve > > rte_flow_item_geneve_mask =3D { #endif > > > > /** > > + * RTE_FLOW_ITEM_TYPE_ARP > > + * > > + * Matches IPv4 ARP packet header >=20 > As above: >=20 > =3D> Matches an IPv4 ARP header. > =3D> RTE_FLOW_ITEM_TYPE_ARP_IPV4 >=20 > > + */ > > +struct rte_flow_item_arp { > > + struct arp_hdr hdr; > > +}; >=20 > Needs #include and a Doxygen comment next to hdr for > consistency, see ICMP and other definitions. >=20 > > + > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */ #ifndef __cplusplus > > +static const struct rte_flow_item_arp rte_flow_item_arp_mask =3D { > > + .hdr =3D { > > + .arp_data =3D { > > + .arp_sha =3D { > > + .addr_bytes =3D "\xff\xff\xff\xff\xff\xff", > > + }, > > + .arp_sip =3D RTE_BE32(0xffffffff), > > + .arp_tha =3D { > > + .addr_bytes =3D "\xff\xff\xff\xff\xff\xff", > > + }, > > + .arp_tip =3D RTE_BE32(0xffffffff), > > + }, > > + }, > > +}; > > +#endif > > + > > +/** > > + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY > > + * > > + * Matches any IPv6 extension header. > > + */ > > +struct rte_flow_item_ipv6_ext_hdr_any { > > + uint8_t next_hdr; > > +}; >=20 > So what's the point? next_hdr is already part of either struct ipv6_hdr > ("proto") and individual extension headers. Moreover it's implicit if an > extension header is provided in a pattern. >=20 > How about removing it? We need this to match a packet that have extend header For example: IPV6(proto =3D 43, ) / EXT_HDR (next_head =3D 60 ) / EXT_HDR (next_head =3D 44, ) / TCP ... I use "ANY" to match any extend header regardless their content. There is no conflict if we can add multiple RTE_FLOW_ITEM_EXT_HDR_XXX in fu= tures >=20 > > + > > +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */ > #ifndef > > +__cplusplus static const struct rte_flow_item_ipv6_ext_hdr_any > > +rte_flow_item_ipv6_ext_any_mask =3D { > > + .next_hdr =3D 0xff, > > +}; > > +#endif >=20 > Ditto. >=20 > > + > > +/** > > + * RTE_FLOW_ITEM_TYPE_ICMPV6 > > + * > > + * Matches ICMPv6 header. >=20 > =3D> Matches an ICMPv6 header. >=20 > > + */ > > +struct rte_flow_item_icmpv6 { > > + uint8_t type; > > + uint8_t code; > > + uint16_t checksum; >=20 > The last 32-bit "reserved" data field is missing. >=20 > > +}; >=20 > Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could = add it. > In any case Doxygen comments are missing, please add them (see other > structure definitions for examples). >=20 > > + > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */ >=20 > Missing "." >=20 > > +#ifndef __cplusplus > > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask =3D= { > > + .type =3D 0xff, > > + .code =3D 0xff, > > + .checksum =3D RTE_BE16(0xffff), > > +}; > > +#endif >=20 > You must remove checksum matching from the default mask. That's the last > thing an application might want to match exactly :) >=20 > > + > > +/** > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR > > + * > > + * Matches ICMPv6's Target Address. > > + */ > > +struct rte_flow_item_icmpv6_tgt_addr { > > + uint8_t addr[16]; > > +}; >=20 > You need to expand this as two items, see prior comments regarding > RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and their > respective structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na. >=20 > Also Doxygen documentation is missing for the addr field and you need to > describe that these are only valid when used after > RTE_FLOW_ITEM_TYPE_ICMPV6. >=20 > > + > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */ >=20 > Missing "." >=20 > > +#ifndef __cplusplus > > +static const > > +struct rte_flow_item_icmpv6_tgt_addr > rte_flow_item_icmpv6_tgt_addr_mask =3D { > > + .addr =3D > > + "\xff\xff\xff\xff\xff\xff\xff\xff" > > + "\xff\xff\xff\xff\xff\xff\xff\xff", > > +}; > > +#endif > > + > > +/** > > + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL. > > + * > > + * Matches ICMPv6 Source Link-Layer address. > > + */ > > +struct rte_flow_item_icmpv6_sll { > > + struct ether_addr addr; > > +}; >=20 > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH and > struct rte_flow_item_type_nd6_opt_sla_eth. >=20 > Also Doxygen documentation is missing for the addr field and you need to > describe that it is only valid when found after either > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA. >=20 > Also missing empty line here. >=20 > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */ >=20 > Missing "." >=20 > > +#ifndef __cplusplus > > +static const struct rte_flow_item_icmpv6_sll > rte_flow_item_icmpv6_sll_mask =3D { > > + .addr =3D { > > + .addr_bytes =3D "\xff\xff\xff\xff\xff\xff", > > + } > > +}; > > +#endif > > + > > +/** > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL. > > + * > > + * Matches ICMPv6 Target Link-Layer address. > > + */ > > +struct rte_flow_item_icmpv6_tll { > > + struct ether_addr addr; > > +}; >=20 > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH > and struct rte_flow_item_type_nd6_opt_tla_eth. >=20 > Also Doxygen documentation is missing for the addr field and you need to > describe that it is only valid when found after either > RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA. >=20 > Also missing empty line here. >=20 > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */ >=20 > Missing "." >=20 > > +#ifndef __cplusplus > > +static const struct rte_flow_item_icmpv6_tll > rte_flow_item_icmpv6_tll_mask =3D { > > + .addr =3D { > > + .addr_bytes =3D "\xff\xff\xff\xff\xff\xff", > > + } > > +}; > > +#endif > > + > > +/** > > * Matching pattern item definition. > > * > > * A pattern is formed by stacking items starting from the lowest > > protocol > > -- > > 2.7.4 > > >=20 > -- > Adrien Mazarguil > 6WIND