diff mbox series

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

Message ID 5d9e2fcd3a1bf439b0cff354ca5b5bf1f43e090d.1562058723.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 Compilation issues

Commit Message

Xiaoyu Min July 2, 2019, 9:45 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 crksv is 0x2000 crksv mask 0xb000 /
	  gre_key key 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.

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
---
** This patch is based on patch [1]

[1] https://patches.dpdk.org/patch/55773/
---
 app/test-pmd/cmdline_flow.c                 | 32 +++++++++++++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
 2 files changed, 36 insertions(+)

Comments

Ori Kam July 2, 2019, 9:53 a.m. UTC | #1 
> -----Original Message-----
> From: Xiaoyu Min <jackmin@mellanox.com>
> Sent: Tuesday, July 2, 2019 12:46 PM
> To: Ori Kam <orika@mellanox.com>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org
> Subject: [Suspected-Phishing][PATCH v4 4/4] app/testpmd: match GRE's key and
> present bits
> 
> 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 crksv is 0x2000 crksv mask 0xb000 /
> 	  gre_key key 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.
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> ---

Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori Kam

> ** This patch is based on patch [1]
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F55773%2F&amp;data=02%7C01%7Corika%40mellanox.co
> m%7C1d141143694542013e2a08d6fed23185%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C636976576061576503&amp;sdata=d3lj4YEdQn96zqvb
> U5VDWvIu40IUFSNAaug51eOSHns%3D&amp;reserved=0
> ---
>  app/test-pmd/cmdline_flow.c                 | 32 +++++++++++++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 201bd9de56..8504cc8bc1 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -148,6 +148,9 @@ enum index {
>  	ITEM_MPLS_LABEL,
>  	ITEM_GRE,
>  	ITEM_GRE_PROTO,
> +	ITEM_GRE_CRKSV,
> +	ITEM_GRE_KEY,
> +	ITEM_GRE_KEY_KEY,
>  	ITEM_FUZZY,
>  	ITEM_FUZZY_THRESH,
>  	ITEM_GTP,
> @@ -595,6 +598,7 @@ static const enum index next_item[] = {
>  	ITEM_NVGRE,
>  	ITEM_MPLS,
>  	ITEM_GRE,
> +	ITEM_GRE_KEY,
>  	ITEM_FUZZY,
>  	ITEM_GTP,
>  	ITEM_GTPC,
> @@ -755,6 +759,13 @@ static const enum index item_mpls[] = {
> 
>  static const enum index item_gre[] = {
>  	ITEM_GRE_PROTO,
> +	ITEM_GRE_CRKSV,
> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
> +static const enum index item_gre_key[] = {
> +	ITEM_GRE_KEY_KEY,
>  	ITEM_NEXT,
>  	ZERO,
>  };
> @@ -1898,6 +1909,27 @@ static const struct token token_list[] = {
>  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
>  					     protocol)),
>  	},
> +	[ITEM_GRE_CRKSV] = {
> +		.name = "crksv",
> +		.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_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_KEY] = {
> +		.name = "key",
> +		.help = "GRE key",
> +		.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..fc3ba8a009 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.
> +
> +  - ``key {unsigned}``: key value.
> +
>  - ``fuzzy``: fuzzy pattern match, expect faster than default.
> 
>    - ``thresh {unsigned}``: accuracy threshold.
> --
> 2.21.0
Adrien Mazarguil July 3, 2019, 3:25 p.m. UTC | #2 
On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> 	  gre_key key 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.
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>

I'm wondering... Is matching the K bit mandatory if one explicitly matches
gre_key already or is this a specific hardware limitation in your case?

Perhaps we could document that the K bit is implicitly matched as "1" in the
default mask when a gre_key pattern item is present. If a user explicitly
spec/mask K as "0" and still provides gre_key, the PMD can safely ignore the
gre_key item.

I'm asking because I think most users won't bother with the K bit when
attempting to match some key and their rules may not behave as expected as a
result.

More below.

> ---
> ** This patch is based on patch [1]
> 
> [1] https://patches.dpdk.org/patch/55773/
> ---
>  app/test-pmd/cmdline_flow.c                 | 32 +++++++++++++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 201bd9de56..8504cc8bc1 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -148,6 +148,9 @@ enum index {
>  	ITEM_MPLS_LABEL,
>  	ITEM_GRE,
>  	ITEM_GRE_PROTO,
> +	ITEM_GRE_CRKSV,
> +	ITEM_GRE_KEY,
> +	ITEM_GRE_KEY_KEY,

Assuming you move the GRE_KEY definition in rte_flow.h, please keep its
location synchronized in this list as well.

>  	ITEM_FUZZY,
>  	ITEM_FUZZY_THRESH,
>  	ITEM_GTP,
> @@ -595,6 +598,7 @@ static const enum index next_item[] = {
>  	ITEM_NVGRE,
>  	ITEM_MPLS,
>  	ITEM_GRE,
> +	ITEM_GRE_KEY,
>  	ITEM_FUZZY,
>  	ITEM_GTP,
>  	ITEM_GTPC,
> @@ -755,6 +759,13 @@ static const enum index item_mpls[] = {
>  
>  static const enum index item_gre[] = {
>  	ITEM_GRE_PROTO,
> +	ITEM_GRE_CRKSV,

CRKSV may be unnecessary in this patch if the K bit is documented and
implemented as described in my previous comment.

> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
> +static const enum index item_gre_key[] = {
> +	ITEM_GRE_KEY_KEY,
>  	ITEM_NEXT,
>  	ZERO,
>  };
> @@ -1898,6 +1909,27 @@ static const struct token token_list[] = {
>  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
>  					     protocol)),
>  	},
> +	[ITEM_GRE_CRKSV] = {
> +		.name = "crksv",
> +		.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_KEY] = {
> +		.name = "gre_key",
> +		.help = "match GRE Key",
> +		.priv = PRIV_ITEM(GRE_KEY,
> +				  sizeof(rte_be32_t)),

Could be a single line.

> +		.next = NEXT(item_gre_key),
> +		.call = parse_vc,
> +	},
> +	[ITEM_GRE_KEY_KEY] = {
> +		.name = "key",
> +		.help = "GRE key",
> +		.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..fc3ba8a009 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.
> +
> +  - ``key {unsigned}``: key value.
> +

You should have named this field "value" then, i.e.:

 - ``value {unsigned}``: key value.

>  - ``fuzzy``: fuzzy pattern match, expect faster than default.
>  
>    - ``thresh {unsigned}``: accuracy threshold.
> -- 
> 2.21.0
>
Xiaoyu Min July 4, 2019, 5:52 a.m. UTC | #3 
On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> > 	  gre_key key 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.
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> 
> I'm wondering... Is matching the K bit mandatory if one explicitly matches
> gre_key already or is this a specific hardware limitation in your case?
> 

If there is gre_key item MLX5 PMD will force set HW matching on K bit,
From HW perspective it is mandatory. But, from testpmd (user)
perspective, I agree with you, user needn't set matching on K bit if
they already explicitly set gre_key item.

> Perhaps we could document that the K bit is implicitly matched as "1" in the
> default mask when a gre_key pattern item is present. If a user explicitly

Yes, I should document this.
So it should be documented in __testpmd_funcs.rst__ ?

> spec/mask K as "0" and still provides gre_key, the PMD can safely ignore the
> gre_key item.
> 

Well, actullay, when a user explicitly set spec/mask K as "0" and still
provide gre_key item, MLX5 PMD will implicitly set match on K bit as
"1", just ingore the K bit set by user.
The reason is wanna keep code simple, needn't to get
information from other item (gre) inside gre_key item, or vice verse.

And, I think, when a user provides a gre_key item, most probably, they do
really wanna match on gre_key. What do you think?

> I'm asking because I think most users won't bother with the K bit when
> attempting to match some key and their rules may not behave as expected as a
> result.
> 

I see.

> More below.
> 
> > ---
> > ** This patch is based on patch [1]
> > 
> > [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.dpdk.org%2Fpatch%2F55773%2F&amp;data=02%7C01%7Cjackmin%40mellanox.com%7C590e61b809bb42869cf508d6ffcaa82c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636977643198683579&amp;sdata=LhTsrHRfX3LHhiHRBtz4WKUUklWupJueSBgWmiHPECM%3D&amp;reserved=0
> > ---
> >  app/test-pmd/cmdline_flow.c                 | 32 +++++++++++++++++++++
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 201bd9de56..8504cc8bc1 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -148,6 +148,9 @@ enum index {
> >  	ITEM_MPLS_LABEL,
> >  	ITEM_GRE,
> >  	ITEM_GRE_PROTO,
> > +	ITEM_GRE_CRKSV,
> > +	ITEM_GRE_KEY,
> > +	ITEM_GRE_KEY_KEY,
> 
> Assuming you move the GRE_KEY definition in rte_flow.h, please keep its
> location synchronized in this list as well.
> 

I'll do this.

> >  	ITEM_FUZZY,
> >  	ITEM_FUZZY_THRESH,
> >  	ITEM_GTP,
> > @@ -595,6 +598,7 @@ static const enum index next_item[] = {
> >  	ITEM_NVGRE,
> >  	ITEM_MPLS,
> >  	ITEM_GRE,
> > +	ITEM_GRE_KEY,
> >  	ITEM_FUZZY,
> >  	ITEM_GTP,
> >  	ITEM_GTPC,
> > @@ -755,6 +759,13 @@ static const enum index item_mpls[] = {
> >  
> >  static const enum index item_gre[] = {
> >  	ITEM_GRE_PROTO,
> > +	ITEM_GRE_CRKSV,
> 
> CRKSV may be unnecessary in this patch if the K bit is documented and
> implemented as described in my previous comment.
> 

Well, actully, we also wanna testpmd can match on C,S bits with K bit
together so we can test on gre packet with key only or csum + key, or
csum + key + sequence.

> > +	ITEM_NEXT,
> > +	ZERO,
> > +};
> > +
> > +static const enum index item_gre_key[] = {
> > +	ITEM_GRE_KEY_KEY,
> >  	ITEM_NEXT,
> >  	ZERO,
> >  };
> > @@ -1898,6 +1909,27 @@ static const struct token token_list[] = {
> >  		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
> >  					     protocol)),
> >  	},
> > +	[ITEM_GRE_CRKSV] = {
> > +		.name = "crksv",
> > +		.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_KEY] = {
> > +		.name = "gre_key",
> > +		.help = "match GRE Key",
> > +		.priv = PRIV_ITEM(GRE_KEY,
> > +				  sizeof(rte_be32_t)),
> 
> Could be a single line.
> 

Yes, I'll update it.

> > +		.next = NEXT(item_gre_key),
> > +		.call = parse_vc,
> > +	},
> > +	[ITEM_GRE_KEY_KEY] = {
> > +		.name = "key",
> > +		.help = "GRE key",
> > +		.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..fc3ba8a009 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.
> > +
> > +  - ``key {unsigned}``: key value.
> > +
> 
> You should have named this field "value" then, i.e.:
> 
>  - ``value {unsigned}``: key value.
> 

OK, I'll update it.

> >  - ``fuzzy``: fuzzy pattern match, expect faster than default.
> >  
> >    - ``thresh {unsigned}``: accuracy threshold.
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Adrien Mazarguil
> 6WIND
Adrien Mazarguil July 4, 2019, 9:52 a.m. UTC | #4 
On Thu, Jul 04, 2019 at 05:52:43AM +0000, Jack Min wrote:
> On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> > On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> > > 	  gre_key key 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.
> > > 
> > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > 
> > I'm wondering... Is matching the K bit mandatory if one explicitly matches
> > gre_key already or is this a specific hardware limitation in your case?
> > 
> 
> If there is gre_key item MLX5 PMD will force set HW matching on K bit,
> From HW perspective it is mandatory. But, from testpmd (user)
> perspective, I agree with you, user needn't set matching on K bit if
> they already explicitly set gre_key item.

OK, makes sense.

> > Perhaps we could document that the K bit is implicitly matched as "1" in the
> > default mask when a gre_key pattern item is present. If a user explicitly
> 
> Yes, I should document this.
> So it should be documented in __testpmd_funcs.rst__ ?

No it would be a change in the GRE_KEY item itself at the rte_flow API
level (rte_flow.h) & documentation (rte_flow.rst). The flow rules created by
testpmd must be an exact translation of user input, as a debugging tool it
can't request something that wasn't explicitly written.

> > spec/mask K as "0" and still provides gre_key, the PMD can safely ignore the
> > gre_key item.
> > 
> 
> Well, actullay, when a user explicitly set spec/mask K as "0" and still
> provide gre_key item, MLX5 PMD will implicitly set match on K bit as
> "1", just ingore the K bit set by user.

Not good then. You should spit an error out if it's an impossible
combination. You can't match both K == 0 *and* a GRE key, unless perhaps if
key mask is also 0, e.g.:

 gre crksv is 0x0000 crksv mask 0xb000 /
 gre_key value spec 0x00000000 value mask 0x00000000

This is merely an overly complex way for telling the PMD that one wants to
match packets without GRE keys that you could technically support.

> The reason is wanna keep code simple, needn't to get
> information from other item (gre) inside gre_key item, or vice verse.

PMDs typically maintain context as they process the pattern. The GRE pattern
item is guaranteed to come before GRE_KEY, so you already know at this point
whether users want to match K at all, and if so, what value they want it to
have.

> And, I think, when a user provides a gre_key item, most probably, they do
> really wanna match on gre_key. What do you think?

Depends. They may want to match all GRE traffic with a key, doesn't matter
which, in order to process it through a different path. To do so they could
either:

1. Use the GRE item only to match K bit == 1.

2. Use the GRE_KEY item to match a nonspecific key value (mask == 0).

3. Use a combination of both.

I think you can easily support all three of them with mlx5 if you support
partial masks on GRE keys (I haven't checked), even if you're unable to
specifically match the K bit itself.

[...]
> > > @@ -755,6 +759,13 @@ static const enum index item_mpls[] = {
> > >  
> > >  static const enum index item_gre[] = {
> > >  	ITEM_GRE_PROTO,
> > > +	ITEM_GRE_CRKSV,
> > 
> > CRKSV may be unnecessary in this patch if the K bit is documented and
> > implemented as described in my previous comment.
> > 
> 
> Well, actully, we also wanna testpmd can match on C,S bits with K bit
> together so we can test on gre packet with key only or csum + key, or
> csum + key + sequence.

OK no problem. Perhaps you could make this easier by allowing users to match
individual bits, let me explain:

The flow command in testpmd is a direct interface to manipulate rte_flow's
structures. The "crksv" field doesn't exist in rte_flow_item_gre, its name
is "c_rsvd0_ver". Testpmd must use the same in its command and internal
code.

However since bit-masks are usually a pain to mentally work out, you can
provide extras for convenience. The "types" field of the RSS action
(ACTION_RSS_TYPES) is an extreme example of this approach.

So I suggest adding ITEM_GRE_C_RSVD0_VER taking a 16-bit value like CRKSV,
and complete it with ITEM_GRE_C_BIT, ITEM_GRE_S_BIT and ITEM_GRE_K_BIT
addressing the individual bits you would like to expose for convenience.

[...]
> > You should have named this field "value" then, i.e.:
> > 
> >  - ``value {unsigned}``: key value.
> > 
> 
> OK, I'll update it.

Please remember to update it in rte_flow.h and documentation as well,
thanks.
Xiaoyu Min July 4, 2019, 11:56 a.m. UTC | #5 
On Thu, 19-07-04, 11:52, Adrien Mazarguil wrote:
> On Thu, Jul 04, 2019 at 05:52:43AM +0000, Jack Min wrote:
> > On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> > > On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> > > > 	  gre_key key 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.
> > > > 
> > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > > 
> > > I'm wondering... Is matching the K bit mandatory if one explicitly matches
> > > gre_key already or is this a specific hardware limitation in your case?
> > > 
> > 
> > If there is gre_key item MLX5 PMD will force set HW matching on K bit,
> > From HW perspective it is mandatory. But, from testpmd (user)
> > perspective, I agree with you, user needn't set matching on K bit if
> > they already explicitly set gre_key item.
> 
> OK, makes sense.
> 
> > > Perhaps we could document that the K bit is implicitly matched as "1" in the
> > > default mask when a gre_key pattern item is present. If a user explicitly
> > 
> > Yes, I should document this.
> > So it should be documented in __testpmd_funcs.rst__ ?
> 
> No it would be a change in the GRE_KEY item itself at the rte_flow API
> level (rte_flow.h) & documentation (rte_flow.rst). The flow rules created by
> testpmd must be an exact translation of user input, as a debugging tool it
> can't request something that wasn't explicitly written.
> 

will update rte_flow.h & rte_flow.rst

> > > spec/mask K as "0" and still provides gre_key, the PMD can safely ignore the
> > > gre_key item.
> > > 
> > 
> > Well, actullay, when a user explicitly set spec/mask K as "0" and still
> > provide gre_key item, MLX5 PMD will implicitly set match on K bit as
> > "1", just ingore the K bit set by user.
> 
> Not good then. You should spit an error out if it's an impossible
> combination. You can't match both K == 0 *and* a GRE key, unless perhaps if
> key mask is also 0, e.g.:
> 
>  gre crksv is 0x0000 crksv mask 0xb000 /
>  gre_key value spec 0x00000000 value mask 0x00000000
> 

Never thought man will wirte thing like this, they don't wanna match
gre_key why put the item there ?
But, since you have raised this example, I'll update PMD part to handle this.

> This is merely an overly complex way for telling the PMD that one wants to
> match packets without GRE keys that you could technically support.
> 
> > The reason is wanna keep code simple, needn't to get
> > information from other item (gre) inside gre_key item, or vice verse.
> 
> PMDs typically maintain context as they process the pattern. The GRE pattern
> item is guaranteed to come before GRE_KEY, so you already know at this point
> whether users want to match K at all, and if so, what value they want it to
> have.
> 

Yes, PMD can know. Just need to add some code.

> > And, I think, when a user provides a gre_key item, most probably, they do
> > really wanna match on gre_key. What do you think?
> 
> Depends. They may want to match all GRE traffic with a key, doesn't matter
> which, in order to process it through a different path. To do so they could
> either:
> 
> 1. Use the GRE item only to match K bit == 1.
> 
> 2. Use the GRE_KEY item to match a nonspecific key value (mask == 0).
> 
> 3. Use a combination of both.
> 
> I think you can easily support all three of them with mlx5 if you support
> partial masks on GRE keys (I haven't checked), even if you're unable to
> specifically match the K bit itself.
> 

Already support this.

> [...]
> > > > @@ -755,6 +759,13 @@ static const enum index item_mpls[] = {
> > > >  
> > > >  static const enum index item_gre[] = {
> > > >  	ITEM_GRE_PROTO,
> > > > +	ITEM_GRE_CRKSV,
> > > 
> > > CRKSV may be unnecessary in this patch if the K bit is documented and
> > > implemented as described in my previous comment.
> > > 
> > 
> > Well, actully, we also wanna testpmd can match on C,S bits with K bit
> > together so we can test on gre packet with key only or csum + key, or
> > csum + key + sequence.
> 
> OK no problem. Perhaps you could make this easier by allowing users to match
> individual bits, let me explain:
> 
> The flow command in testpmd is a direct interface to manipulate rte_flow's
> structures. The "crksv" field doesn't exist in rte_flow_item_gre, its name
> is "c_rsvd0_ver". Testpmd must use the same in its command and internal
> code.
> 
> However since bit-masks are usually a pain to mentally work out, you can
> provide extras for convenience. The "types" field of the RSS action
> (ACTION_RSS_TYPES) is an extreme example of this approach.
> 
> So I suggest adding ITEM_GRE_C_RSVD0_VER taking a 16-bit value like CRKSV,
> and complete it with ITEM_GRE_C_BIT, ITEM_GRE_S_BIT and ITEM_GRE_K_BIT
> addressing the individual bits you would like to expose for convenience.
> 

So something like:
  eth / ipv4 / gre c_rsvd0_ver c_bit is 0 s_bit is 0 k_bit is 1 / ...

Is it right?

> [...]
> > > You should have named this field "value" then, i.e.:
> > > 
> > >  - ``value {unsigned}``: key value.
> > > 
> > 
> > OK, I'll update it.
> 
> Please remember to update it in rte_flow.h and documentation as well,
> thanks.
> 
OK.
> -- 
> Adrien Mazarguil
> 6WIND
Adrien Mazarguil July 4, 2019, 12:13 p.m. UTC | #6 
On Thu, Jul 04, 2019 at 11:56:35AM +0000, Jack Min wrote:
> On Thu, 19-07-04, 11:52, Adrien Mazarguil wrote:
> > On Thu, Jul 04, 2019 at 05:52:43AM +0000, Jack Min wrote:
> > > On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> > > > On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> > > > > 	  gre_key key 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.
> > > > > 
> > > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
[...]
> > > Well, actullay, when a user explicitly set spec/mask K as "0" and still
> > > provide gre_key item, MLX5 PMD will implicitly set match on K bit as
> > > "1", just ingore the K bit set by user.
> > 
> > Not good then. You should spit an error out if it's an impossible
> > combination. You can't match both K == 0 *and* a GRE key, unless perhaps if
> > key mask is also 0, e.g.:
> > 
> >  gre crksv is 0x0000 crksv mask 0xb000 /
> >  gre_key value spec 0x00000000 value mask 0x00000000
> > 
> 
> Never thought man will wirte thing like this, they don't wanna match
> gre_key why put the item there ?
> But, since you have raised this example, I'll update PMD part to handle this.

It's just an example of valid yet convoluted command mind you, I'm not
forcing you to support it, however if you don't, you must raise an error,
you can't just ignore the K bit if user provides GRE_KEY.

[...]
> > Depends. They may want to match all GRE traffic with a key, doesn't matter
> > which, in order to process it through a different path. To do so they could
> > either:
> > 
> > 1. Use the GRE item only to match K bit == 1.
> > 
> > 2. Use the GRE_KEY item to match a nonspecific key value (mask == 0).
> > 
> > 3. Use a combination of both.
> > 
> > I think you can easily support all three of them with mlx5 if you support
> > partial masks on GRE keys (I haven't checked), even if you're unable to
> > specifically match the K bit itself.
> > 
> 
> Already support this.

OK, nice.

[...]
> > > Well, actully, we also wanna testpmd can match on C,S bits with K bit
> > > together so we can test on gre packet with key only or csum + key, or
> > > csum + key + sequence.
> > 
> > OK no problem. Perhaps you could make this easier by allowing users to match
> > individual bits, let me explain:
> > 
> > The flow command in testpmd is a direct interface to manipulate rte_flow's
> > structures. The "crksv" field doesn't exist in rte_flow_item_gre, its name
> > is "c_rsvd0_ver". Testpmd must use the same in its command and internal
> > code.
> > 
> > However since bit-masks are usually a pain to mentally work out, you can
> > provide extras for convenience. The "types" field of the RSS action
> > (ACTION_RSS_TYPES) is an extreme example of this approach.
> > 
> > So I suggest adding ITEM_GRE_C_RSVD0_VER taking a 16-bit value like CRKSV,
> > and complete it with ITEM_GRE_C_BIT, ITEM_GRE_S_BIT and ITEM_GRE_K_BIT
> > addressing the individual bits you would like to expose for convenience.
> > 
> 
> So something like:
>   eth / ipv4 / gre c_rsvd0_ver c_bit is 0 s_bit is 0 k_bit is 1 / ...
> 
> Is it right?

Looks like "c_rsvd0_ver" is incomplete, I assume you meant:

 eth / ipv4 / gre c_rsvd0_ver is 0 c_bit is 0 s_bit is 0 k_bit is 1 / ...

And yes it's valid. Of course since nothing is matched by default, users
will typically not provide c_rsvd0_ver at all and focus on the relevant bits
for their use case: 

 eth / ipv4 / gre k_bit is 1 / ...

Another suggestion, use BOOLEAN instead of INTEGER type for C/K/S to support
other binary expressions:

 eth / ipv4 / gre k_bit is on / ...
Xiaoyu Min July 4, 2019, 1:01 p.m. UTC | #7 
On Thu, 19-07-04, 14:13, Adrien Mazarguil wrote:
> On Thu, Jul 04, 2019 at 11:56:35AM +0000, Jack Min wrote:
> > On Thu, 19-07-04, 11:52, Adrien Mazarguil wrote:
> > > On Thu, Jul 04, 2019 at 05:52:43AM +0000, Jack Min wrote:
> > > > On Wed, 19-07-03, 17:25, Adrien Mazarguil wrote:
> > > > > On Tue, Jul 02, 2019 at 05:45:55PM +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 crksv is 0x2000 crksv mask 0xb000 /
> > > > > > 	  gre_key key 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.
> > > > > > 
> > > > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> [...]
> > > > Well, actullay, when a user explicitly set spec/mask K as "0" and still
> > > > provide gre_key item, MLX5 PMD will implicitly set match on K bit as
> > > > "1", just ingore the K bit set by user.
> > > 
> > > Not good then. You should spit an error out if it's an impossible
> > > combination. You can't match both K == 0 *and* a GRE key, unless perhaps if
> > > key mask is also 0, e.g.:
> > > 
> > >  gre crksv is 0x0000 crksv mask 0xb000 /
> > >  gre_key value spec 0x00000000 value mask 0x00000000
> > > 
> > 
> > Never thought man will wirte thing like this, they don't wanna match
> > gre_key why put the item there ?
> > But, since you have raised this example, I'll update PMD part to handle this.
> 
> It's just an example of valid yet convoluted command mind you, I'm not
> forcing you to support it, however if you don't, you must raise an error,
> you can't just ignore the K bit if user provides GRE_KEY.
> 
OK, make sense.
> [...]
> > > Depends. They may want to match all GRE traffic with a key, doesn't matter
> > > which, in order to process it through a different path. To do so they could
> > > either:
> > > 
> > > 1. Use the GRE item only to match K bit == 1.
> > > 
> > > 2. Use the GRE_KEY item to match a nonspecific key value (mask == 0).
> > > 
> > > 3. Use a combination of both.
> > > 
> > > I think you can easily support all three of them with mlx5 if you support
> > > partial masks on GRE keys (I haven't checked), even if you're unable to
> > > specifically match the K bit itself.
> > > 
> > 
> > Already support this.
> 
> OK, nice.
> 
> [...]
> > > > Well, actully, we also wanna testpmd can match on C,S bits with K bit
> > > > together so we can test on gre packet with key only or csum + key, or
> > > > csum + key + sequence.
> > > 
> > > OK no problem. Perhaps you could make this easier by allowing users to match
> > > individual bits, let me explain:
> > > 
> > > The flow command in testpmd is a direct interface to manipulate rte_flow's
> > > structures. The "crksv" field doesn't exist in rte_flow_item_gre, its name
> > > is "c_rsvd0_ver". Testpmd must use the same in its command and internal
> > > code.
> > > 
> > > However since bit-masks are usually a pain to mentally work out, you can
> > > provide extras for convenience. The "types" field of the RSS action
> > > (ACTION_RSS_TYPES) is an extreme example of this approach.
> > > 
> > > So I suggest adding ITEM_GRE_C_RSVD0_VER taking a 16-bit value like CRKSV,
> > > and complete it with ITEM_GRE_C_BIT, ITEM_GRE_S_BIT and ITEM_GRE_K_BIT
> > > addressing the individual bits you would like to expose for convenience.
> > > 
> > 
> > So something like:
> >   eth / ipv4 / gre c_rsvd0_ver c_bit is 0 s_bit is 0 k_bit is 1 / ...
> > 
> > Is it right?
> 
> Looks like "c_rsvd0_ver" is incomplete, I assume you meant:
> 
>  eth / ipv4 / gre c_rsvd0_ver is 0 c_bit is 0 s_bit is 0 k_bit is 1 / ...
> 
> And yes it's valid. Of course since nothing is matched by default, users
> will typically not provide c_rsvd0_ver at all and focus on the relevant bits
> for their use case: 
> 
>  eth / ipv4 / gre k_bit is 1 / ...
> 
> Another suggestion, use BOOLEAN instead of INTEGER type for C/K/S to support
> other binary expressions:
> 
>  eth / ipv4 / gre k_bit is on / ...
>
OK~ 
> -- 
> 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..8504cc8bc1 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -148,6 +148,9 @@  enum index {
 	ITEM_MPLS_LABEL,
 	ITEM_GRE,
 	ITEM_GRE_PROTO,
+	ITEM_GRE_CRKSV,
+	ITEM_GRE_KEY,
+	ITEM_GRE_KEY_KEY,
 	ITEM_FUZZY,
 	ITEM_FUZZY_THRESH,
 	ITEM_GTP,
@@ -595,6 +598,7 @@  static const enum index next_item[] = {
 	ITEM_NVGRE,
 	ITEM_MPLS,
 	ITEM_GRE,
+	ITEM_GRE_KEY,
 	ITEM_FUZZY,
 	ITEM_GTP,
 	ITEM_GTPC,
@@ -755,6 +759,13 @@  static const enum index item_mpls[] = {
 
 static const enum index item_gre[] = {
 	ITEM_GRE_PROTO,
+	ITEM_GRE_CRKSV,
+	ITEM_NEXT,
+	ZERO,
+};
+
+static const enum index item_gre_key[] = {
+	ITEM_GRE_KEY_KEY,
 	ITEM_NEXT,
 	ZERO,
 };
@@ -1898,6 +1909,27 @@  static const struct token token_list[] = {
 		.args = ARGS(ARGS_ENTRY_HTON(struct rte_flow_item_gre,
 					     protocol)),
 	},
+	[ITEM_GRE_CRKSV] = {
+		.name = "crksv",
+		.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_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_KEY] = {
+		.name = "key",
+		.help = "GRE key",
+		.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..fc3ba8a009 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.
+
+  - ``key {unsigned}``: key value.
+
 - ``fuzzy``: fuzzy pattern match, expect faster than default.
 
   - ``thresh {unsigned}``: accuracy threshold.