From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Qi Z" Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API Date: Thu, 12 Apr 2018 10:00:32 +0000 Message-ID: <039ED4275CED7440929022BC67E706115319047A@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> <039ED4275CED7440929022BC67E7061153190216@SHSMSX103.ccr.corp.intel.com> <20180412091931.GP4957@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" To: Adrien Mazarguil Return-path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id B36751BD2D for ; Thu, 12 Apr 2018 12:00:36 +0200 (CEST) In-Reply-To: <20180412091931.GP4957@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: Thursday, April 12, 2018 5:20 PM > 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 Thu, Apr 12, 2018 at 05:12:08AM +0000, Zhang, Qi Z wrote: > > 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 >=20 > Thanks, replying inline also. >=20 > > > -----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 > > > > > > 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 > > > > > > While properly defined in the patch, "IPV6" is missing here. > > > > > > > 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 > > > > > > First, since they are added at the end of enum rte_flow_item_type, > > > no ABI breakage notice is necessary. > > > > > > 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. > > > > > > > > More comments below regarding these definitions. > > > > > > [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an > > > example 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 > > > > > > > --- > > > > 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. > > > > > > =3D> Matches an IPv4 ARP header. > > > > > > > + * > > > > + * See struct rte_flow_item_arp. > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_ARP, > > > > > > 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 has both a fixed and a variably-sized part. > > > > > > Ideally an ARP pattern item should match the fixed part only and a > > > separate > > > ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP > below. > > > > > > 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= : > > > > > > =3D> RTE_FLOW_ITEM_TYPE_ARP_IPV4 > > > > > > > + > > > > + /** > > > > + * Matches any IPv6 Extension header. > > > > > > =3D> Matches an IPv6 extension header. > > > > > > > + * > > > > + * See struct rte_flow_item_ipv6_ext_any. > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY, > > > > > > I'm not sure this definition is necessary, more below about that. > > > > > > Also I don't see a benefit in having "ANY" part of the name, if you > > > want to keep it, I suggest the simpler: > > > > > > =3D> RTE_FLOW_ITEM_TYPE_IPV6_EXT > > > > > > > + > > > > + /** > > > > + * Matches ICMPv6 header. > > > > > > =3D> Matches an ICMPv6 header. > > > > > > > + * > > > > + * See struct rte_flow_item_icmpv6 > > > > > > Missing "." > > > > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_ICMPV6, > > > > + > > > > > > 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 be named after the NDP types they represent, not = inner > data fields. > > > > > > Also I think we should consider NDP as a protocol sitting on top of > > > ICMPv6. We could therefore drop "ICMP" from these definitions. > > > > > > 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 related pattern items. > > > > I agree. > > > > > > > > These are the reasons behind my next suggestions: > > > > > > > + /** > > > > + * Match ICMPv6 target address. > > > > + * > > > > + * See struct rte_flow_item_icmpv6_tgt_addr. > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR, > > > > > > =3D> Matches an IPv6 network discovery router solicitation. > > > =3D> See struct rte_flow_item_nd6_rs. > > > =3D> RTE_FLOW_ITEM_TYPE_ND6_RS, >=20 > By the way, I wrote "router solicitation" (RS) here but it should have be= en > "neighbor solicitation" (NS) obviously. >=20 > > > > > > You should add another item for neighbor advertisement messages > > > using the same template: > > > > > > =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 address" > > 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 > > Solicitation) 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 rationale is that while they share a similar format, they are in fact= different > messages that applications could want to match more conveniently than > providing ICMP type/code values. It would be done for consistency given t= he > same RFC also defines router solicitation/advertisement messages. >=20 > However a problem remains since these messages are part of the ICMP forma= t > whose "reserved" field sometimes contains message flags, particularly wit= h RA. > These structures would lack that data. >=20 > Honestly your approach makes sense, but it shouldn't be possible to mix t= arget > addresses with RA/RS and it should be convenient to match these messages > without specifically matching their contents. >=20 > So another suggestion would be to define new types at the ICMPv6 level to= use > directly on top of ETH for each possible message and define separate > structures for options. >=20 > First let's drop one character here and in all other definitions in this > patch: >=20 > ICMPV6 =3D> ICMP6 >=20 > Then the new items would respectively be: >=20 > RTE_FLOW_ITEM_TYPE_ICMP6 > RTE_FLOW_ITEM_TYPE_ICMP6_ND_NA > RTE_FLOW_ITEM_TYPE_ICMP6_ND_NS > RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_SLA > RTE_FLOW_ITEM_TYPE_ICMP6_ND_OPT_TLA >=20 > All the related structure definitions would include the ICMPv6 header par= t > defined according to the RFC and except for RTE_FLOW_ITEM_TYPE_ICMP6, a > default mask that excludes type/code since they are implicit: >=20 > struct rte_flow_item_icmp6_nd_na { > uint8_t type; /**< ICMPv6 type, normally 136. */ > uint8_t code; /**< ICMPv6 code, normally 0. */ > rte_be16_t checksum; /**< ICMPv6 checksum. */ > /** > * Router flag (1b), solicited flag (1b), override flag (1b), > * reserved (29b). > */ > rte_be32_t rso_reserved; > uint8_t target[16]; /**< Target address. */ }; >=20 > static const struct rte_flow_item_icmp6_nd_na > rte_flow_item_icmp6_nd_na_mask =3D { > .target =3D > "\xff\xff\xff\xff\xff\xff\xff\xff" > "\xff\xff\xff\xff\xff\xff\xff\xff", > }; >=20 > Also notice how uint(16|32)_t were modified as rte_be(16|32)_t while ther= e. >=20 > What's your opinion? OK, I will take this method, it looks good, thanks=20 >=20 > > > > > > > > The following are possible options for these headers, if specified > > > they must 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: > > > > > > > + > > > > + /** > > > > + * Match ICMPv6 Source Link-Layer Address. > > > > + * > > > > + * See struct rte_flow_item_icmpv6_sll. > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_ICMPV6_SLL, > > > > > > =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, > > > > > > > + > > > > + /** > > > > + * Match ICMPv6 Target Link-Layer Address. > > > > + * > > > > + * See struct rte_flow_item_icmpv6_tll. > > > > + */ > > > > + RTE_FLOW_ITEM_TYPE_ICMPV6_TLL, > > > > > > =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, > > > > > > > Agree to rename. > > > > > > + > > > > > > Unnecessary empty line. > > > > > > > }; > > > > > > > > /** > > > > @@ -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 > > > > > > As above: > > > > > > =3D> Matches an IPv4 ARP header. > > > =3D> RTE_FLOW_ITEM_TYPE_ARP_IPV4 > > > > > > > + */ > > > > +struct rte_flow_item_arp { > > > > + struct arp_hdr hdr; > > > > +}; > > > > > > Needs #include and a Doxygen comment next to hdr for > > > consistency, see ICMP and other definitions. > > > > > > > + > > > > +/** 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; > > > > +}; > > > > > > 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. > > > > > > 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 futures >=20 > I see, makes sense. How about doing like ICMPv6 above? Generic item uses = the > base name and can only match the generic part specifically (next_hdr), wh= ile > specific items don't match the generic part but whatever additions their > dedicated structures define, i.e.: >=20 > RTE_FLOW_ITEM_TYPE_IPV6_EXT > RTE_FLOW_ITEM_TYPE_IPV6_EXT_HBH > RTE_FLOW_ITEM_TYPE_IPV6_EXT_DEST > RTE_FLOW_ITEM_TYPE_IPV6_EXT_RTHDR > RTE_FLOW_ITEM_TYPE_IPV6_EXT_FRAG > ... Yes, agree. >=20 > No need to define them all if you only need EXT, this is just to describe= the idea > (it's also OK if you want to define them while you're at it). >=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 > > > > > > Ditto. > > > > > > > + > > > > +/** > > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6 > > > > + * > > > > + * Matches ICMPv6 header. > > > > > > =3D> Matches an ICMPv6 header. > > > > > > > + */ > > > > +struct rte_flow_item_icmpv6 { > > > > + uint8_t type; > > > > + uint8_t code; > > > > + uint16_t checksum; > > > > > > The last 32-bit "reserved" data field is missing. > > > > > > > +}; > > > > > > Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You co= uld > add it. > > > In any case Doxygen comments are missing, please add them (see other > > > structure definitions for examples). >=20 > No need to rely on an external definition due to the above suggestions by= the > way. >=20 > > > > > > > + > > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */ > > > > > > Missing "." > > > > > > > +#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 > > > > > > You must remove checksum matching from the default mask. That's the > > > last thing an application might want to match exactly :) > > > > > > > + > > > > +/** > > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR > > > > + * > > > > + * Matches ICMPv6's Target Address. > > > > + */ > > > > +struct rte_flow_item_icmpv6_tgt_addr { > > > > + uint8_t addr[16]; > > > > +}; > > > > > > 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. > > > > > > 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. > > > > > > > + > > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */ > > > > > > Missing "." > > > > > > > +#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; > > > > +}; > > > > > > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH > and > > > struct rte_flow_item_type_nd6_opt_sla_eth. > > > > > > 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. > > > > > > Also missing empty line here. > > > > > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */ > > > > > > Missing "." > > > > > > > +#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; > > > > +}; > > > > > > See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH > > > and struct rte_flow_item_type_nd6_opt_tla_eth. > > > > > > 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. > > > > > > Also missing empty line here. > > > > > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */ > > > > > > Missing "." > > > > > > > +#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