diff mbox series

[v6,4/4] app/testpmd: match GRE's key and present bits

Message ID c162b5bba3015a468f372337e7655966588a51ef.1562292465.git.jackmin@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series match on GRE's key |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

Xiaoyu Min July 5, 2019, 2:14 a.m. UTC 
  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>
---
 app/test-pmd/cmdline_flow.c                 | 61 +++++++++++++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 ++
 2 files changed, 65 insertions(+)

Comments

Adrien Mazarguil July 5, 2019, 8:58 a.m. UTC | #1 
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.
Xiaoyu Min July 5, 2019, 9:06 a.m. UTC | #2 
On Fri, 19-07-05, 10:58, Adrien Mazarguil wrote:
> 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:
> 
Didn't know this before.

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

> > +		.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)".
> 
I'll update.

> > +		.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)"
> 
Ditto.

> > +		.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)"
> 
Ditto.

> > +		.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"
> 
OK.

> > +		.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"
> 
OK.

> > +		.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.
> 
Yes, it should be. 

> Then assuming all the suggested changes are made:
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
Thank you!

> Note I did not look at mlx5 patches, please make sure someone has reviewed
> them. Thanks.
> 
Yes, Slava will review them.

> -- 
> Adrien Mazarguil
> 6WIND
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 201bd9de56..8a579b645e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -148,6 +148,10 @@  enum index {
 	ITEM_MPLS_LABEL,
 	ITEM_GRE,
 	ITEM_GRE_PROTO,
+	ITEM_GRE_C_RSVD0_VER,
+	ITEM_GRE_C_BIT,
+	ITEM_GRE_K_BIT,
+	ITEM_GRE_S_BIT,
 	ITEM_FUZZY,
 	ITEM_FUZZY_THRESH,
 	ITEM_GTP,
@@ -181,6 +185,8 @@  enum index {
 	ITEM_ICMP6_ND_OPT_TLA_ETH_TLA,
 	ITEM_META,
 	ITEM_META_DATA,
+	ITEM_GRE_KEY,
+	ITEM_GRE_KEY_VALUE,
 
 	/* Validate/create actions. */
 	ACTIONS,
@@ -610,6 +616,7 @@  static const enum index next_item[] = {
 	ITEM_ICMP6_ND_OPT_SLA_ETH,
 	ITEM_ICMP6_ND_OPT_TLA_ETH,
 	ITEM_META,
+	ITEM_GRE_KEY,
 	ZERO,
 };
 
@@ -755,6 +762,16 @@  static const enum index item_mpls[] = {
 
 static const enum index item_gre[] = {
 	ITEM_GRE_PROTO,
+	ITEM_GRE_C_RSVD0_VER,
+	ITEM_GRE_C_BIT,
+	ITEM_GRE_K_BIT,
+	ITEM_GRE_S_BIT,
+	ITEM_NEXT,
+	ZERO,
+};
+
+static const enum index item_gre_key[] = {
+	ITEM_GRE_KEY_VALUE,
 	ITEM_NEXT,
 	ZERO,
 };
@@ -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)",
+		.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",
+		.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",
+		.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",
+		.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",
+		.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",
+		.next = NEXT(item_gre_key, NEXT_ENTRY(UNSIGNED), item_param),
+		.args = ARGS(ARG_ENTRY_HTON(rte_be32_t)),
+	},
 	[ITEM_FUZZY] = {
 		.name = "fuzzy",
 		.help = "fuzzy pattern match, expect faster than default",
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index cb83a3ce8a..9ef1796ee1 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3804,6 +3804,10 @@  This section lists supported pattern items and their attributes, if any.
 
   - ``protocol {unsigned}``: protocol type.
 
+- ``gre_key``: match GRE optional key field.
+
+  - ``value {unsigned}``: key value.
+
 - ``fuzzy``: fuzzy pattern match, expect faster than default.
 
   - ``thresh {unsigned}``: accuracy threshold.