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