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

[Jack Min <jackmin@mellanox.com>]

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