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 <jack...@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?
>
>>
>>> + (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
>>>
>>
