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