Re: [dpdk-dev] [PATCH v2 3/4] ether: add more protocol support in flow API

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 12, 2018 5:20 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 3/4] ether: add more protocol support in flow API
> 
> 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.mazarguil@6wind.com]
> > > Sent: Thursday, April 12, 2018 12:32 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@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.zhang@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?

OK, I will take this method, it looks good, thanks 

> 
> >
> > >
> > > 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
>  ...

Yes, agree.

> 
> 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