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 <jack...@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
