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

Adrien Mazarguil adrien.mazarguil at 6wind.com
Wed Apr 11 18:32:13 CEST 2018


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

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.

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

> +

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?

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


More information about the dev mailing list