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 <or...@mellanox.com> > > Signed-off-by: Xiaoyu Min <jack...@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.mazarg...@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