From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9D8DAA034F; Wed, 6 May 2020 11:36:13 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0BC0E1D72C; Wed, 6 May 2020 11:36:09 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id C05281D71C for ; Wed, 6 May 2020 11:36:07 +0200 (CEST) IronPort-SDR: /tbFbJ072eIZ9M85kIzJyA69tp9q3qaHqghC5pMjcDJUAPxPCY+9iLUF4yZqU2/BuniF0+zvTH 30IM7iIO44Hg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 May 2020 02:36:06 -0700 IronPort-SDR: 2loO09+SODiRsW1mQz4s+cNM/rU/bfOGoCV3wZdZuW8V0mRzucpC6bHqVsoasvWBm5RFVHKsAA kbepclHkAiMQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,358,1583222400"; d="scan'208";a="461372102" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.213.197.179]) ([10.213.197.179]) by fmsmga005.fm.intel.com with ESMTP; 06 May 2020 02:36:04 -0700 To: Xiaoyu Min Cc: Wenzhuo Lu , Jingjing Wu , Bernard Iremonger , John McNamara , Marko Kovacevic , dev@dpdk.org References: <127483b3dbf4840a80c6177a6230d1f3e8f40fb2.1588251701.git.jackmin@mellanox.com> <8a562aef-6d7b-4713-7fb9-e4da12d6bf56@intel.com> <20200506025247.psjk3deaitbwa6pd@mellanox.com> From: Ferruh Yigit Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJsBBMBCgBWAhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEABQkKqZZ8FiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl6ha3sXGHZrczovL2tl eXMub3BlbnBncC5vcmcACgkQ+TPrQ98TYR8uLA//QwltuFliUWe60xwmu9sY38c1DXvX67wk UryQ1WijVdIoj4H8cf/s2KtyIBjc89R254KMEfJDao/LrXqJ69KyGKXFhFPlF3VmFLsN4XiT PSfxkx8s6kHVaB3O183p4xAqnnl/ql8nJ5ph9HuwdL8CyO5/7dC/MjZ/mc4NGq5O9zk3YRGO lvdZAp5HW9VKW4iynvy7rl3tKyEqaAE62MbGyfJDH3C/nV/4+mPc8Av5rRH2hV+DBQourwuC ci6noiDP6GCNQqTh1FHYvXaN4GPMHD9DX6LtT8Fc5mL/V9i9kEVikPohlI0WJqhE+vQHFzR2 1q5nznE+pweYsBi3LXIMYpmha9oJh03dJOdKAEhkfBr6n8BWkWQMMiwfdzg20JX0o7a/iF8H 4dshBs+dXdIKzPfJhMjHxLDFNPNH8zRQkB02JceY9ESEah3wAbzTwz+e/9qQ5OyDTQjKkVOo cxC2U7CqeNt0JZi0tmuzIWrfxjAUulVhBmnceqyMOzGpSCQIkvalb6+eXsC9V1DZ4zsHZ2Mx Hi+7pCksdraXUhKdg5bOVCt8XFmx1MX4AoV3GWy6mZ4eMMvJN2hjXcrreQgG25BdCdcxKgqp e9cMbCtF+RZax8U6LkAWueJJ1QXrav1Jk5SnG8/5xANQoBQKGz+yFiWcgEs9Tpxth15o2v59 gXK5Ag0EV9ZMvgEQAKc0Db17xNqtSwEvmfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ES YpV8QWj0xK4YM0dLxnDU2IYxjEshSB1TqAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4Ai bPtrHuIXWQOBECcVZTTOdZYGAzaYzxiAONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxD UQljeNvKYt1lZE/gAUUxNLWsYyTT+22/vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/ 3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35piVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVj sM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQI3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdc q9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYHfVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH7 1PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZqw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFB VOQOxCvwRG2QCgcJ/UTn5vlivul+cThi6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI 8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJlRr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYC GwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNhHwUCXqFrngUJCKxSYAAKCRD5M+tD3xNhH3YWD/9b cUiWaHJasX+OpiuZ1Li5GG3m9aw4lR/k2lET0UPRer2Jy1JsL+uqzdkxGvPqzFTBXgx/6Byz EMa2mt6R9BCyR286s3lxVS5Bgr5JGB3EkpPcoJT3A7QOYMV95jBiiJTy78Qdzi5LrIu4tW6H o0MWUjpjdbR01cnj6EagKrDx9kAsqQTfvz4ff5JIFyKSKEHQMaz1YGHyCWhsTwqONhs0G7V2 0taQS1bGiaWND0dIBJ/u0pU998XZhmMzn765H+/MqXsyDXwoHv1rcaX/kcZIcN3sLUVcbdxA WHXOktGTQemQfEpCNuf2jeeJlp8sHmAQmV3dLS1R49h0q7hH4qOPEIvXjQebJGs5W7s2vxbA 5u5nLujmMkkfg1XHsds0u7Zdp2n200VC4GQf8vsUp6CSMgjedHeF9zKv1W4lYXpHp576ZV7T GgsEsvveAE1xvHnpV9d7ZehPuZfYlP4qgo2iutA1c0AXZLn5LPcDBgZ+KQZTzm05RU1gkx7n gL9CdTzVrYFy7Y5R+TrE9HFUnsaXaGsJwOB/emByGPQEKrupz8CZFi9pkqPuAPwjN6Wonokv ChAewHXPUadcJmCTj78Oeg9uXR6yjpxyFjx3vdijQIYgi5TEGpeTQBymLANOYxYWYOjXk+ae dYuOYKR9nbPv+2zK9pwwQ2NXbUBystaGyQ== Message-ID: Date: Wed, 6 May 2020 10:36:03 +0100 MIME-Version: 1.0 In-Reply-To: <20200506025247.psjk3deaitbwa6pd@mellanox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: add cmdline option to set Rx mq mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>> --- >>> 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 >>> >>