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
Thanks, replying inline also. > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > Sent: Thursday, April 12, 2018 12:32 AM > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > Cc: dev@dpdk.org; Doherty, Declan <declan.dohe...@intel.com>; Chandran, > > Sugesh <sugesh.chand...@intel.com>; Glynn, Michael J > > <michael.j.gl...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; Ananyev, > > Konstantin <konstantin.anan...@intel.com>; Richardson, Bruce > > <bruce.richard...@intel.com> > > 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 <qi.z.zh...@intel.com> > > > > 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. > > > > => 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: > > > > => RTE_FLOW_ITEM_TYPE_ARP_IPV4 > > > > > + > > > + /** > > > + * Matches any IPv6 Extension header. > > > > => 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: > > > > => RTE_FLOW_ITEM_TYPE_IPV6_EXT > > > > > + > > > + /** > > > + * Matches ICMPv6 header. > > > > => 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, > > > > => Matches an IPv6 network discovery router solicitation. > > => See struct rte_flow_item_nd6_rs. > > => RTE_FLOW_ITEM_TYPE_ND6_RS, By the way, I wrote "router solicitation" (RS) here but it should have been "neighbor solicitation" (NS) obviously. > > > > You should add another item for neighbor advertisement messages using the > > same template: > > > > => Match an IPv6 network discovery neighbor advertisement. > > => See struct rte_flow_item_nd6_na. > > => 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 = 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? 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 the same RFC also defines router solicitation/advertisement messages. However a problem remains since these messages are part of the ICMP format whose "reserved" field sometimes contains message flags, particularly with RA. These structures would lack that data. Honestly your approach makes sense, but it shouldn't be possible to mix target addresses with RA/RS and it should be convenient to match these messages without specifically matching their contents. 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. First let's drop one character here and in all other definitions in this patch: ICMPV6 => ICMP6 Then the new items would respectively be: 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 All the related structure definitions would include the ICMPv6 header part defined according to the RFC and except for RTE_FLOW_ITEM_TYPE_ICMP6, a default mask that excludes type/code since they are implicit: 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. */ }; static const struct rte_flow_item_icmp6_nd_na rte_flow_item_icmp6_nd_na_mask = { .target = "\xff\xff\xff\xff\xff\xff\xff\xff" "\xff\xff\xff\xff\xff\xff\xff\xff", }; Also notice how uint(16|32)_t were modified as rte_be(16|32)_t while there. What's your opinion? > > > > > 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, > > > > => Matches an IPv6 network discovery source Ethernet link-layer address > > option. > > => See struct rte_flow_item_nd6_opt_sla_eth. > > => 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, > > > > => Matches an IPv6 network discovery target Ethernet link-layer address > > option. > > => See struct rte_flow_item_nd6_opt_tla_eth. > > => 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 = { #endif > > > > > > /** > > > + * RTE_FLOW_ITEM_TYPE_ARP > > > + * > > > + * Matches IPv4 ARP packet header > > > > As above: > > > > => Matches an IPv4 ARP header. > > => RTE_FLOW_ITEM_TYPE_ARP_IPV4 > > > > > + */ > > > +struct rte_flow_item_arp { > > > + struct arp_hdr hdr; > > > +}; > > > > Needs #include <rte_arp.h> 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 = { > > > + .hdr = { > > > + .arp_data = { > > > + .arp_sha = { > > > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > > > + }, > > > + .arp_sip = RTE_BE32(0xffffffff), > > > + .arp_tha = { > > > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > > > + }, > > > + .arp_tip = 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 = 43, <Routing EH >) / EXT_HDR (next_head = 60 <Destination EH>) / > EXT_HDR (next_head = 44, <Fragment EH)/ EXT_HDR (next_head = 6 <tcp>) / 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 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), while specific items don't match the generic part but whatever additions their dedicated structures define, i.e.: 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 ... 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). > > > > > > + > > > +/** 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 = { > > > + .next_hdr = 0xff, > > > +}; > > > +#endif > > > > Ditto. > > > > > + > > > +/** > > > + * RTE_FLOW_ITEM_TYPE_ICMPV6 > > > + * > > > + * Matches ICMPv6 header. > > > > => 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 could > > add it. > > In any case Doxygen comments are missing, please add them (see other > > structure definitions for examples). No need to rely on an external definition due to the above suggestions by the way. > > > > > + > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */ > > > > Missing "." > > > > > +#ifndef __cplusplus > > > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask = { > > > + .type = 0xff, > > > + .code = 0xff, > > > + .checksum = 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 = { > > > + .addr = > > > + "\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 = { > > > + .addr = { > > > + .addr_bytes = "\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 = { > > > + .addr = { > > > + .addr_bytes = "\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 > > > -- Adrien Mazarguil 6WIND