Hi Jeff,
> -----Original Message-----
> From: Guo, Jia <jia....@intel.com>
> Sent: Thursday, April 16, 2020 8:52 AM
> To: Iremonger, Bernard <bernard.iremon...@intel.com>;
> or...@mellanox.com; Ye, Xiaolong <xiaolong...@intel.com>; Zhang, Qi Z
> <qi.z.zh...@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing...@intel.com>; Cao, Yahui
> <yahui....@intel.com>; Su, Simei <simei...@intel.com>
> Subject: Re: [dpdk-dev v5 3/3] app/testpmd: add new types to RSS hash
> commands
>
> hi, bernard
>
>
> On 4/15/2020 11:01 PM, Iremonger, Bernard wrote:
> > Hi Jeff,
> >
> >> -----Original Message-----
> >> From: Guo, Jia <jia....@intel.com>
> >> Sent: Wednesday, April 15, 2020 6:11 PM
> >> To: Iremonger, Bernard <bernard.iremon...@intel.com>;
> >> or...@mellanox.com; Ye, Xiaolong <xiaolong...@intel.com>; Zhang, Qi Z
> >> <qi.z.zh...@intel.com>
> >> Cc: dev@dpdk.org; Wu, Jingjing <jingjing...@intel.com>; Cao, Yahui
> >> <yahui....@intel.com>; Su, Simei <simei...@intel.com>; Guo, Jia
> >> <jia....@intel.com>
> >> Subject: [dpdk-dev v5 3/3] app/testpmd: add new types to RSS hash
> >> commands
> >>
> >> Add some new types, such as eth/l2-src-only/l2-dst-only/svlan/cvlan/
> >> l2tpv3/esp/ah/pfcp types into RSS hash commands, it could be used to
> >> configure these rss input set by cmdline.
> >>
> >> Example testpmd commands are:
> >> Eth:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / end actions rss \
> >> types eth l2-src-only end key_len 0 queues end / end
> >>
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / end actions rss \
> >> types eth l2-dst-only end key_len 0 queues end / end
> >>
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / end actions rss \
> >> types eth end key_len 0 queues end / end
> >>
> >> s-vlan:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / end actions rss \
> >> types c-vlan end key_len 0 queues end / end
> >>
> >> c-vlan:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / end actions rss \
> >> types c-vlan end key_len 0 queues end / end
> >>
> >> l2tpv3:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / l2tpv3oip / end \
> >> actions rss types ipv4 l2tpv3 end key_len 0 queues end / end
> >>
> >> esp:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / esp / end actions
> >> testpmd>\
> >> rss types ipv4 esp end key_len 0 queues end / end
> > Should "rss types ipv4 esp" be "rss types esp" ?
>
>
> yes, it should be.
>
>
> >> ah:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / ah / end actions \
> >> rss types ipv4 ah end key_len 0 queues end / end
> >>
> > Should "rss types ah" be "rss types ah" ?
>
>
> yes, the same as above and i think you are right, all action should be
> specific,
> i think the parse logic should be modify to use pattern to distinguish the
> ipv4
> ah or ipv6 ah but not use rss type.
>
>
> >> pfcp:
> >> testpmd>flow create 0 ingress pattern eth / ipv4 / udp / pfcp / end \
> >> actions rss types ipv4-udp pfcp end key_len 0 queues end / end
> > The above sample commands might be better in a doc file, rather than in
> the commit message.
> > For example doc/guides/howto/rte_flow.rst
> >
> > The flow create/validate commands are normally parsed in
> > app/testpmd/cmdline_flow.c This patch is updating the following
> commands:
> > "port config all rss all"
> > "port config <port_id> rss-hash-key"
>
>
> Since this is not the command initial and just leverage the command which is
> already exist, so i think i would suppose that no need to modify the doc if
> no
> change, only change
>
> the part if the change bring in, and i could briefly introduce a example only
> in
> commit log for simplify.
I still think it would be useful to add the examples above to the
doc/guides/howto/rte_flow.rst file which contains other rte_flow examples.
>
>
> >> Signed-off-by: Jeff Guo <jia....@intel.com>
> >> ---
> >> v5->v4:
> >> rename eth to l2 and refine commit log
> >> ---
> >> app/test-pmd/cmdline.c | 28 +++++++++++++++++++++++++---
> app/test-
> >> pmd/config.c | 9 +++++++++
> >> 2 files changed, 34 insertions(+), 3 deletions(-)
> >
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >> 863b567c1..6b688ab66 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -2270,9 +2270,11 @@ cmd_config_rss_parsed(void *parsed_result,
> >> int ret;
> >>
> >> if (!strcmp(res->value, "all"))
> >> -rss_conf.rss_hf = ETH_RSS_IP | ETH_RSS_TCP |
> >> +rss_conf.rss_hf = ETH_RSS_ETH | ETH_RSS_IP |
> >> ETH_RSS_TCP |
> >> ETH_RSS_UDP | ETH_RSS_SCTP |
> >> ETH_RSS_L2_PAYLOAD;
> >> +else if (!strcmp(res->value, "eth"))
> >> +rss_conf.rss_hf = ETH_RSS_ETH;
> >> else if (!strcmp(res->value, "ip"))
> >> rss_conf.rss_hf = ETH_RSS_IP;
> >> else if (!strcmp(res->value, "udp"))
> >> @@ -2299,6 +2301,22 @@ cmd_config_rss_parsed(void *parsed_result,
> >> rss_conf.rss_hf = ETH_RSS_L4_SRC_ONLY;
> >> else if (!strcmp(res->value, "l4-dst-only"))
> >> rss_conf.rss_hf = ETH_RSS_L4_DST_ONLY;
> >> +else if (!strcmp(res->value, "l2-src-only"))
> >> +rss_conf.rss_hf = ETH_RSS_L2_SRC_ONLY;
> >> +else if (!strcmp(res->value, "l2-dst-only"))
> >> +rss_conf.rss_hf = ETH_RSS_L2_DST_ONLY;
> >> +else if (!strcmp(res->value, "s-vlan"))
> >> +rss_conf.rss_hf = ETH_RSS_S_VLAN;
> >> +else if (!strcmp(res->value, "c-vlan"))
> >> +rss_conf.rss_hf = ETH_RSS_C_VLAN;
> >> +else if (!strcmp(res->value, "l2tpv3"))
> >> +rss_conf.rss_hf = ETH_RSS_L2TPV3;
> >> +else if (!strcmp(res->value, "esp"))
> >> +rss_conf.rss_hf = ETH_RSS_ESP;
> >> +else if (!strcmp(res->value, "ah"))
> >> +rss_conf.rss_hf = ETH_RSS_AH;
> >> +else if (!strcmp(res->value, "pfcp"))
> >> +rss_conf.rss_hf = ETH_RSS_PFCP;
> >> else if (!strcmp(res->value, "none"))
> >> rss_conf.rss_hf = 0;
> >> else if (!strcmp(res->value, "default")) @@ -2467,7 +2485,9 @@
> >> cmdline_parse_token_string_t cmd_config_rss_hash_key_rss_type =
> >> "ipv4-other#ipv6#ipv6-frag#ipv6-tcp#ipv6-
> >> udp#"
> >> "ipv6-sctp#ipv6-other#l2-payload#ipv6-ex#"
> >> "ipv6-tcp-ex#ipv6-udp-ex#"
> >> - "l3-src-only#l3-dst-only#l4-src-only#l4-dst-
> >> only");
> >> + "l3-src-only#l3-dst-only#l4-src-only#l4-dst-
> >> only#"
> >> + "l2-src-only#l2-dst-only#s-vlan#c-vlan#"
> >> + "l2tpv3#esp#ah#pfcp");
> >> cmdline_parse_token_string_t cmd_config_rss_hash_key_value =
> >> TOKEN_STRING_INITIALIZER(struct cmd_config_rss_hash_key, key,
> >> NULL);
> >>
> >> @@ -2478,7 +2498,9 @@ cmdline_parse_inst_t
> cmd_config_rss_hash_key =
> >> {
> >> "ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
> >> "ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
> >> "l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex|"
> >> -"l3-src-only|l3-dst-only|l4-src-only|l4-dst-only "
> >> +"l3-src-only|l3-dst-only|l4-src-only|l4-dst-only|"
> >> +"l2-src-only|l2-dst-only|s-vlan|c-vlan|"
> >> +"l2tpv3|esp|ah|pfcp "
> >> "<string of hex digits (variable length, NIC dependent)>",
> >> .tokens = {
> >> (void *)&cmd_config_rss_hash_key_port, diff --git
> >> a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >> 71aeb5413..e4a97388b 100644
> >> --- a/app/test-pmd/config.c
> >> +++ b/app/test-pmd/config.c
> >> @@ -79,6 +79,11 @@ const struct rss_type_info rss_type_table[] = {
> >> ETH_RSS_UDP | ETH_RSS_SCTP |
> >> ETH_RSS_L2_PAYLOAD },
> >> { "none", 0 },
> >> +{ "eth", ETH_RSS_ETH },
> >> +{ "l2-src-only", ETH_RSS_L2_SRC_ONLY },
> >> +{ "l2-dst-only", ETH_RSS_L2_DST_ONLY },
> >> +{ "s-vlan", ETH_RSS_S_VLAN },
> >> +{ "c-vlan", ETH_RSS_C_VLAN },
> > Should these additions be at the bottom of the array with the other
> additions ?
>
>
> I suppose this is not in a library code or application binary interface,
> so it would not break ABI/API, and re-order it as up-down layer must be
> better.
Ok.
>
>
> >> { "ipv4", ETH_RSS_IPV4 },
> >> { "ipv4-frag", ETH_RSS_FRAG_IPV4 },
> >> { "ipv4-tcp", ETH_RSS_NONFRAG_IPV4_TCP }, @@ -108,6 +113,10
> >> @@ const struct rss_type_info rss_type_table[] = {
> >> { "l3-dst-only", ETH_RSS_L3_DST_ONLY },
> >> { "l4-src-only", ETH_RSS_L4_SRC_ONLY },
> >> { "l4-dst-only", ETH_RSS_L4_DST_ONLY },
> >> +{ "l2tpv3", ETH_RSS_L2TPV3 },
> >> +{ "esp", ETH_RSS_ESP },
> >> +{ "ah", ETH_RSS_AH },
> >> +{ "pfcp", ETH_RSS_PFCP },
> >> { NULL, 0 },
> >> };
> >>
> >> --
> >> 2.20.1
Regards,
Bernard.
