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

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

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

> 
> > +
> > +/** 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).
> 
> > +
> > +/** 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