Re: [dpdk-dev] [PATCH v2] app/testpmd: add cmdline option to set Rx mq mode

[Xiaoyu Min <jackmin@mellanox.com>]

On Wed, 20-05-06, 10:36, Ferruh Yigit wrote:
> On 5/6/2020 3:52 AM, Xiaoyu Min wrote:
> > On Tue, 20-05-05, 17:06, Ferruh Yigit wrote:
> >> On 4/30/2020 2:07 PM, Xiaoyu Min wrote:
> >>> One new cmdline option `--rx-mq-mode` is added in order to have the
> >>> possibility to check whether PMD handle the mq mode correctly or not.
> >>>
> >>> The reason is some NICs need to do different settings based on different
> >>> RX mq mode, i.e RSS or not.
> >>>
> >>> With this support in testpmd, the above scenario can be tested easily.
> >>>
> >>> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> >>> ---
> >>>  app/test-pmd/parameters.c              | 12 ++++++++++++
> >>>  app/test-pmd/testpmd.c                 | 17 ++++++++++++++---
> >>>  app/test-pmd/testpmd.h                 |  3 +++
> >>>  doc/guides/rel_notes/release_20_05.rst |  4 ++++
> >>>  doc/guides/testpmd_app_ug/run_app.rst  |  7 +++++++
> >>>  5 files changed, 40 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> >>> index 30c1753c32..a9dd58e96b 100644
> >>> --- a/app/test-pmd/parameters.c
> >>> +++ b/app/test-pmd/parameters.c
> >>> @@ -212,6 +212,7 @@ usage(char* progname)
> >>>  	printf("  --noisy-lkup-num-writes=N: do N random reads and writes per packet\n");
> >>>  	printf("  --no-iova-contig: mempool memory can be IOVA non contiguous. "
> >>>  	       "valid only with --mp-alloc=anon\n");
> >>> +	printf("  --rx-mq-mode=0xX: hexadecimal bitmask of RX mq mode\n");
> >>
> >> Do you think does it worth to say the bitmask is for modes that can be enabled,
> >> to remove need to look the code, not sure.
> > So you mean something like:
> > +	printf("  --rx-mq-mode=0xX: hexadecimal bitmask of RX mq mode can be enabled\n");
> > Is it right?
> 
> Yes.
> 
> > 
> >>
> >>>  }
> >>>  
> >>>  #ifdef RTE_LIBRTE_CMDLINE
> >>> @@ -670,6 +671,7 @@ launch_args_parse(int argc, char** argv)
> >>>  		{ "noisy-lkup-num-reads",	1, 0, 0 },
> >>>  		{ "noisy-lkup-num-reads-writes", 1, 0, 0 },
> >>>  		{ "no-iova-contig",             0, 0, 0 },
> >>> +		{ "rx-mq-mode",                 1, 0, 0 },
> >>>  		{ 0, 0, 0, 0 },
> >>>  	};
> >>>  
> >>> @@ -1363,6 +1365,16 @@ launch_args_parse(int argc, char** argv)
> >>>  			}
> >>>  			if (!strcmp(lgopts[opt_idx].name, "no-iova-contig"))
> >>>  				mempool_flags = MEMPOOL_F_NO_IOVA_CONTIG;
> >>> +
> >>> +			if (!strcmp(lgopts[opt_idx].name, "rx-mq-mode")) {
> >>> +				char *end = NULL;
> >>> +				n = strtoul(optarg, &end, 16);
> >>> +				if (n >= 0)
> >>> +					rx_mq_mode = (enum rte_eth_rx_mq_mode)n;
> >>
> >> Should we check if the provided value, 'n', is not out of the enum range?
> > OK, I'll add the check.
> > 
> >>
> >>> +				else
> >>> +					rte_exit(EXIT_FAILURE,
> >>> +						 "rx-mq-mode must be >= 0\n");
> >>> +			}
> >>>  			break;
> >>>  		case 'h':
> >>>  			usage(argv[0]);
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >>> index 99bacddbfd..9536d6ee27 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -482,6 +482,11 @@ uint8_t bitrate_enabled;
> >>>  struct gro_status gro_ports[RTE_MAX_ETHPORTS];
> >>>  uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES;
> >>>  
> >>> +/*
> >>> + * RX mq mode value set in the commandline
> >>
> >> This is not the "RX mq mode value", the above help string seems more accurate,
> >> "hexadecimal bitmask of RX mq mode". Can you please update here?
> > Sure, I'll update.
> > 
> >>
> >>> + */
> >>> +enum rte_eth_rx_mq_mode rx_mq_mode = ETH_MQ_RX_VMDQ_DCB_RSS;
> >>> +
> >>>  /* Forward function declarations */
> >>>  static void setup_attached_port(portid_t pi);
> >>>  static void map_port_queue_stats_mapping_registers(portid_t pi,
> >>> @@ -3337,7 +3342,9 @@ init_port_config(void)
> >>>  
> >>>  		if (port->dcb_flag == 0) {
> >>>  			if( port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
> >>> -				port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_RSS;
> >>> +				port->dev_conf.rxmode.mq_mode =
> >>> +					(enum rte_eth_rx_mq_mode)
> >>
> >> Do we need this enum type cast?
> > Yes, we need this cast otherwise the coverity will complain about enum type
> > mixed with other type.
> 
> Here all variables looks an enum, 'mq_mode' type is enum, 'rx_mq_mode' is enum
> and 'ETH_MQ_RX_RSS' is enum item. With which type it is mixed with?
> 
> I didn't run coverity for it, if it is giving warning OK to keep them, but can
> you please double check with coverity scan?
> 
As you asked, I double checked with coverity by removing the enum cast. The coverity
has the below warning [1] but this time the warning is supressed by filter(I guess
it's newly added). It seems coverity doesn't allow the bitwise operators(?)
Anyhow I prefer to keep the cast this time since it's already there.
What do you think?

[1]:
[2020-05-06T10:31:01.899Z] Detected 1 defect occurrence that passes the filter criteria.
[2020-05-06T10:31:01.899Z] There are 267 suppressed defects due to filters.
[2020-05-06T10:31:01.900Z] 
[2020-05-06T10:31:01.900Z] app/test-pmd/testpmd.c:3345: CID 154009 (#1 of 1):
[2020-05-06T10:31:01.901Z]   Type: Parse warning (PW.MIXED_ENUM_TYPE)
[2020-05-06T10:31:01.901Z]   Classification: Unclassified
[2020-05-06T10:31:01.901Z]   Severity: Unspecified
[2020-05-06T10:31:01.902Z]   Action: Undecided
[2020-05-06T10:31:01.902Z]   Owner: Unassigned
[2020-05-06T10:31:01.903Z]   Defect only exists locally.
[2020-05-06T10:31:01.903Z] app/test-pmd/testpmd.c:3345:
[2020-05-06T10:31:01.903Z]   1. mixed_enum_type: enumerated type mixed with another type
> > 
> >>
> >>> +						(rx_mq_mode & ETH_MQ_RX_RSS);
> >>>  			else
> >>>  				port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
> >>>  		}
> >>> @@ -3438,7 +3445,9 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
> >>>  		}
> >>>  
> >>>  		/* set DCB mode of RX and TX of multiple queues */
> >>> -		eth_conf->rxmode.mq_mode = ETH_MQ_RX_VMDQ_DCB;
> >>> +		eth_conf->rxmode.mq_mode =
> >>> +				(enum rte_eth_rx_mq_mode)
> >>> +					(rx_mq_mode & ETH_MQ_RX_VMDQ_DCB);
> >>>  		eth_conf->txmode.mq_mode = ETH_MQ_TX_VMDQ_DCB;
> >>>  	} else {
> >>>  		struct rte_eth_dcb_rx_conf *rx_conf =
> >>> @@ -3458,7 +3467,9 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
> >>>  			tx_conf->dcb_tc[i] = i % num_tcs;
> >>>  		}
> >>>  
> >>> -		eth_conf->rxmode.mq_mode = ETH_MQ_RX_DCB_RSS;
> >>> +		eth_conf->rxmode.mq_mode =
> >>> +				(enum rte_eth_rx_mq_mode)
> >>> +					(rx_mq_mode & ETH_MQ_RX_DCB_RSS);
> >>>  		eth_conf->rx_adv_conf.rss_conf = rss_conf;
> >>>  		eth_conf->txmode.mq_mode = ETH_MQ_TX_DCB;
> >>>  	}
> >>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> >>> index 7ff4c5dba3..32bb324c94 100644
> >>> --- a/app/test-pmd/testpmd.h
> >>> +++ b/app/test-pmd/testpmd.h
> >>> @@ -602,6 +602,9 @@ struct mplsoudp_decap_conf {
> >>>  };
> >>>  extern struct mplsoudp_decap_conf mplsoudp_decap_conf;
> >>>  
> >>> +/* RX mq mode parameter. */
> >>
> >> The variable name gives as much context as the comment, may it be dropped?
> > OK, I'll dorp this comment.
> > 
> >>
> >>> +extern enum rte_eth_rx_mq_mode rx_mq_mode;
> >>> +
> >>>  static inline unsigned int
> >>>  lcore_num(void)
> >>>  {
> >>> diff --git a/doc/guides/rel_notes/release_20_05.rst b/doc/guides/rel_notes/release_20_05.rst
> >>> index b124c3f287..8026afb451 100644
> >>> --- a/doc/guides/rel_notes/release_20_05.rst
> >>> +++ b/doc/guides/rel_notes/release_20_05.rst
> >>> @@ -212,6 +212,10 @@ New Features
> >>>    * Added IPsec inbound load-distribution support for ipsec-secgw application
> >>>      using NIC load distribution feature(Flow Director).
> >>>  
> >>> +* **Updated testpmd application.**
> >>> +
> >>> +  * Added a new cmdline option ``--rx-mq-mode`` which can be used to test PMD's
> >>> +    behaviour on handling Rx mq mode.
> >>>  
> >>>  Removed Items
> >>>  -------------
> >>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> >>> index 727ef52b8f..4f46299e68 100644
> >>> --- a/doc/guides/testpmd_app_ug/run_app.rst
> >>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> >>> @@ -481,3 +481,10 @@ The command line options are:
> >>>  
> >>>      Enable to create mempool which is not IOVA contiguous. Valid only with --mp-alloc=anon.
> >>>      The default value is 0.
> >>> +
> >>> +*   ``--rx-mq-mode``
> >>> +
> >>> +    Set the hexadecimal bitmask of RX queue mq mode.
> >>
> >> It is good to expand the 'mq' at least in the documentation, and I guess it is
> >> "multi queue".
> > OK, I'll expand it.
> > 
> > Thank you
> > -Jack
> >>
> >>> +    The default value is 0x7::
> >>> +
> >>> +       ETH_MQ_RX_RSS_FLAG | ETH_MQ_RX_DCB_FLAG | ETH_MQ_RX_VMDQ_FLAG
> >>>
> >>
>