Re: [dpdk-dev] [dpdk-dev v5 3/3] app/testpmd: add new types to RSS hash commands

[Jeff Guo <jia.guo@intel.com>]

hi, bernard


On 4/16/2020 5:14 PM, Iremonger, Bernard wrote:
> Hi Jeff,
>
>> -----Original Message-----
>> From: Guo, Jia <jia.guo@intel.com>
>> Sent: Thursday, April 16, 2020 8:52 AM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>;
>> orika@mellanox.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Cao, Yahui
>> <yahui.cao@intel.com>; Su, Simei <simei.su@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.guo@intel.com>
>>>> Sent: Wednesday, April 15, 2020 6:11 PM
>>>> To: Iremonger, Bernard <bernard.iremonger@intel.com>;
>>>> orika@mellanox.com; Ye, Xiaolong <xiaolong.ye@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>
>>>> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Cao, Yahui
>>>> <yahui.cao@intel.com>; Su, Simei <simei.su@intel.com>; Guo, Jia
>>>> <jia.guo@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.


I think what your suppose is that there is lack of an doc to describe 
this rss rule feature what ever pf and vf in the history, so i suppose 
to add it in a separate patch next

after leverage all pf and vf about that, what is your or other guys 
opinion, i think i am both ok for that.


>>
>>>> Signed-off-by: Jeff Guo <jia.guo@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.