> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, April 25, 2018 8:04 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; adrien.mazarg...@6wind.com
> Cc: tho...@monjalon.net; dev@dpdk.org
> Subject: Re: [PATCH v2] app/testpmd: fix testpmd failure due to RSS offload
> check
>
> On 4/25/2018 8:43 AM, Qi Zhang wrote:
> > After add RSS hash offload check, default rss_hf will fail on devices
> > that not support all bits, the patch introduce RSS negotiate flag,
> > when it is set, RSS configuration will negotiate with device
> > capability. By default negotiate is turn on, so it will not break
> > exist PMD. It can be disabled by "--disable-rss-neg"
> > in start parameters or be enable/disable by testpmd command "port
> > config all rss neg|noneg".
>
> Hi Qi,
>
> Thanks for the patch, but I feel new neg|noneg is extra complexity, I am
> wondering if we can fix this without this complexity.
>
> "rss_hf" is rss hash function configuration for *all* ports. Default value is
> ETH_RSS_IP.
>
> Up until recently it was more like default value, Adrien's commit start
> updating it in "port config all rss ..." and it become more like config value
> now.
>
> And if it is config value, the question is what to do if PMD doesn't support
> requested hash function:
> - Fail
> - Convert request what PMD supports.
>
> init_port_config() called from many places and it sets rss_conf.rss_hf via
> "rss_hf" and currently this is the problem some pmds hit.
>
> Previously, before Adrien's update, when "port config all rss ..." failed it
> was
> printing log and keep continue, not recording the failed value as config
> value.
>
>
> What do you think:
> - In init_port_config() use "rss_hf & dev_info.flow_type_rss_offloads" as you
> suggested.
> - in cmd_config_rss_parsed() set "rss_hf" only if all ethdev successfully
> called
> rte_eth_dev_rss_hash_update(), this will mean the config has supported by
> all ports so it will be ok to use is as "rss_hf &
> dev_info.flow_type_rss_offloads"
> in init_port_config()
> - I think there is a defect in "port config all rss default", perhaps because
> of
> me during merged, it shouldn't update "rss_hf"
>
> After this changes answer to above question, by default config behavior is
> "Convert request what PMD supports" so testpmd don't fail, and when
> configuration updated only values supported by ports accepted.
Ok, thanks for share the background, I'm ok with your suggestion, please check
v3.
Regards
Qi
>
>
>
> >
> > Fixes: 527624c663f8 ("ethdev: add supported hash function check")
> > Signed-off-by: Qi Zhang <qi.z.zh...@intel.com>
> > ---
> >
> > v2:
> > - fix command help string.
> >
> > app/test-pmd/cmdline.c | 22
> ++++++++++++++++------
> > app/test-pmd/parameters.c | 5 +++++
> > app/test-pmd/testpmd.c | 12 +++++++++++-
> > app/test-pmd/testpmd.h | 1 +
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 +++-
> > 5 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 333852194..9390be741 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -822,7 +822,7 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> > " for ports.\n\n"
> >
> > "port config all rss (all|default|ip|tcp|udp|sctp|"
> > - "ether|port|vxlan|geneve|nvgre|none|<flowtype_id>)\n"
> > +
> "ether|port|vxlan|geneve|nvgre|none|neg|noneg|<flowtype_id>)\n"
> > " Set the RSS mode.\n\n"
> >
> > "port config port-id rss reta
> > (hash,queue)[,(hash,queue)]\n"
> > @@ -2029,7 +2029,13 @@ cmd_config_rss_parsed(void *parsed_result,
> > else if (isdigit(res->value[0]) && atoi(res->value) > 0 &&
> > atoi(res->value) < 64)
> > rss_conf.rss_hf = 1ULL << atoi(res->value);
> > - else {
> > + else if (!strcmp(res->value, "neg")) {
> > + rss_hf_noneg = 0;
> > + return;
> > + } else if (!strcmp(res->value, "noneg")) {
> > + rss_hf_noneg = 1;
> > + return;
> > + } else {
> > printf("Unknown parameter\n");
> > return;
> > }
> > @@ -2037,10 +2043,14 @@ cmd_config_rss_parsed(void *parsed_result,
> > /* Update global configuration for RSS types. */
> > rss_hf = rss_conf.rss_hf;
> > RTE_ETH_FOREACH_DEV(i) {
> > - if (!strcmp(res->value, "default")) {
> > - rte_eth_dev_info_get(i, &dev_info);
> > + rte_eth_dev_info_get(i, &dev_info);
> > + if (!strcmp(res->value, "default"))
> > rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
> > - }
> > + else
> > + rss_conf.rss_hf &= (rss_hf_noneg ?
> > + rss_conf.rss_hf :
> > + dev_info.flow_type_rss_offloads);
> > +
> > diag = rte_eth_dev_rss_hash_update(i, &rss_conf);
> > if (diag < 0)
> > printf("Configuration of RSS hash at ethernet port %d "
> > @@ -2064,7 +2074,7 @@ cmdline_parse_inst_t cmd_config_rss = {
> > .f = cmd_config_rss_parsed,
> > .data = NULL,
> > .help_str = "port config all rss "
> > -
> "all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|<fl
> owtype_id>",
> > +
> >
> +"all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|neg|n
> > +oneg|<flowtype_id>",
> > .tokens = {
> > (void *)&cmd_config_rss_port,
> > (void *)&cmd_config_rss_keyword,
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index 394fa6d92..4758ec1d9 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -139,6 +139,7 @@ usage(char* progname)
> > printf(" --enable-hw-vlan-extend: enable hardware vlan extend.\n");
> > printf(" --enable-drop-en: enable per queue packet drop.\n");
> > printf(" --disable-rss: disable rss.\n");
> > + printf(" --disable-rss-neg: disable rss negotiate with device
> > +capa.\n");
> > printf(" --port-topology=N: set port topology (N: paired (default) or "
> > "chained).\n");
> > printf(" --forward-mode=N: set forwarding mode (N: %s).\n", @@
> > -594,6 +595,7 @@ launch_args_parse(int argc, char** argv)
> > { "enable-hw-vlan-extend", 0, 0, 0 },
> > { "enable-drop-en", 0, 0, 0 },
> > { "disable-rss", 0, 0, 0 },
> > + { "disable-rss-neg", 0, 0, 0 },
> > { "port-topology", 1, 0, 0 },
> > { "forward-mode", 1, 0, 0 },
> > { "rss-ip", 0, 0, 0 },
> > @@ -909,6 +911,9 @@ launch_args_parse(int argc, char** argv)
> >
> > if (!strcmp(lgopts[opt_idx].name, "disable-rss"))
> > rss_hf = 0;
> > + if (!strcmp(lgopts[opt_idx].name, "disable-rss-neg")) {
> > + rss_hf_noneg = 1;
> > + }
> > if (!strcmp(lgopts[opt_idx].name, "port-topology")) {
> > if (!strcmp(optarg, "paired"))
> > port_topology = PORT_TOPOLOGY_PAIRED;
> > diff --git
> > a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > d6da41927..68214677c 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -257,6 +257,12 @@ int16_t tx_rs_thresh = RTE_PMD_PARAM_UNSET;
> > uint64_t rss_hf = ETH_RSS_IP; /* RSS IP by default. */
> >
> > /*
> > + * Negotiate with device capability when config
> > + * Receive Side Scaling (RSS).
> > + */
> > +uint8_t rss_hf_noneg = 0; /* Negotiate by default */
> > +
> > +/*
> > * Port topology configuration
> > */
> > uint16_t port_topology = PORT_TOPOLOGY_PAIRED; /* Ports are paired
> by
> > default */ @@ -2265,13 +2271,17 @@ init_port_config(void) {
> > portid_t pid;
> > struct rte_port *port;
> > + struct rte_eth_dev_info dev_info;
> >
> > RTE_ETH_FOREACH_DEV(pid) {
> > port = &ports[pid];
> > port->dev_conf.fdir_conf = fdir_conf;
> > if (nb_rxq > 1) {
> > + rte_eth_dev_info_get(pid, &dev_info);
> > port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> > - port->dev_conf.rx_adv_conf.rss_conf.rss_hf = rss_hf;
> > + port->dev_conf.rx_adv_conf.rss_conf.rss_hf =
> > + rss_hf_noneg ? rss_hf :
> > + (rss_hf & dev_info.flow_type_rss_offloads);
> > } else {
> > port->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
> > port->dev_conf.rx_adv_conf.rss_conf.rss_hf = 0; diff
> > --git
> > a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > 070919822..c37c4ee70 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -385,6 +385,7 @@ extern struct rte_eth_rxmode rx_mode; extern
> > struct rte_eth_txmode tx_mode;
> >
> > extern uint64_t rss_hf;
> > +extern uint8_t rss_hf_noneg;
> >
> > extern queueid_t nb_rxq;
> > extern queueid_t nb_txq;
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index 593b13a3d..8db9cf5c1 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -1751,13 +1751,15 @@ port config - RSS
> >
> > Set the RSS (Receive Side Scaling) mode on or off::
> >
> > - testpmd> port config all rss
> (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none)
> > + testpmd> port config all rss
> > +
> (all|default|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none|neg|
> > + noneg)
> >
> > RSS is on by default.
> >
> > The ``all`` option is equivalent to ip|tcp|udp|sctp|ether.
> > The ``default`` option enables all supported RSS types reported by device
> info.
> > The ``none`` option is equivalent to the ``--disable-rss`` command-line
> option.
> > +The ``neg`` option enable the negotiation with device capability when
> configure RSS.
> > +The ``noneg`` option disable the negotiation and is equivalent to
> ``--disable-rss-neg`` command-line option.
> >
> > port config - RSS Reta
> > ~~~~~~~~~~~~~~~~~~~~~~
> >
