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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 5/6/2020 5:04 PM, Xiaoyu Min wrote:
> 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?

OK, thanks for re-checking.

> 
> [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
>>>>>
>>>>
>>