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