Re: [dpdk-dev] [PATCH v6 4/4] app/testpmd: match GRE's key and present bits

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Fri, Jul 05, 2019 at 10:14:45AM +0800, Xiaoyu Min wrote:
> support matching on GRE key and present bits (C,K,S)
> 
> example testpmd command could be:
>   testpmd>flow create 0 ingress group 1 pattern eth / ipv4 /
> 	  gre / gre_key value is 0x12345678 / end
> 	  actions rss queues 1 0 end / mark id 196 / end
> 
> Which will match GRE packet with k present bit set and key value is
> 0x12345678.
> 
> Acked-by: Ori Kam <orika@mellanox.com>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>

A few more nits below.

[...]
> @@ -1898,6 +1915,50 @@ static const struct token token_list[] = {
>  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
>  					     protocol)),
>  	},
> +	[ITEM_GRE_C_RSVD0_VER] = {
> +		.name = "c_rsvd0_ver",
> +		.help = "GRE's first word (bit0 - bit15)",

Help strings on existing fields should ideally be the same as their
counterparts in rte_flow.h (shortened if necessary, not starting with a cap
and not ending "."), in this case for instance:

 .help =
         "checksum (1b), undefined (1b), key bit (1b),"
         " sequence number (1b), reserved 0 (9b),"
         " version (3b)",

> +		.next = NEXT(item_gre, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
> +					     c_rsvd0_ver)),
> +	},
> +	[ITEM_GRE_C_BIT] = {
> +		.name = "c_bit",
> +		.help = "GRE's C present bit",

A bit odd, here's a suggestion:

 "checksum bit (C)".

> +		.next = NEXT(item_gre, NEXT_ENTRY(BOOLEAN), item_param),
> +		.args = ARGS(ARGS_ENTRY_MASK_HTON(struct rte_flow_item_gre,
> +						  c_rsvd0_ver,
> +						  "\x80\x00\x00\x00")),
> +	},
> +	[ITEM_GRE_S_BIT] = {
> +		.name = "s_bit",
> +		.help = "GRE's S present bit",

Ditto:

 "sequence number bit (S)"

> +		.next = NEXT(item_gre, NEXT_ENTRY(BOOLEAN), item_param),
> +		.args = ARGS(ARGS_ENTRY_MASK_HTON(struct rte_flow_item_gre,
> +						  c_rsvd0_ver,
> +						  "\x10\x00\x00\x00")),
> +	},
> +	[ITEM_GRE_K_BIT] = {
> +		.name = "k_bit",
> +		.help = "GRE's K present bit",

Ditto:

 "key bit (K)"

> +		.next = NEXT(item_gre, NEXT_ENTRY(BOOLEAN), item_param),
> +		.args = ARGS(ARGS_ENTRY_MASK_HTON(struct rte_flow_item_gre,
> +						  c_rsvd0_ver,
> +						  "\x20\x00\x00\x00")),
> +	},
> +	[ITEM_GRE_KEY] = {
> +		.name = "gre_key",
> +		.help = "match GRE Key",

Nit: no caps for "Key" => "match GRE key"

> +		.priv = PRIV_ITEM(GRE_KEY, sizeof(rte_be32_t)),
> +		.next = NEXT(item_gre_key),
> +		.call = parse_vc,
> +	},
> +	[ITEM_GRE_KEY_VALUE] = {
> +		.name = "value",
> +		.help = "GRE key value",

No need to repeat "GRE" here since it's already in GRE context:

 "key value"

> +		.next = NEXT(item_gre_key, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARG_ENTRY_HTON(rte_be32_t)),
> +	},

Also ITEM_GRE_KEY and ITEM_GRE_KEY_VALUE should come after ITEM_META_DATA to
keep the same order as everywhere else.

Then assuming all the suggested changes are made:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Note I did not look at mlx5 patches, please make sure someone has reviewed
them. Thanks.

-- 
Adrien Mazarguil
6WIND