Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Friday, January 09, 2015 9:50 PM
To: Ouyang, Changchun; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/09/15 07:54, Ouyang, Changchun wrote:





-----Original Message-----

From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]

Sent: Friday, January 9, 2015 2:49 AM

To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@dpdk.org>

Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode





On 01/08/15 11:19, Vlad Zolotarov wrote:



On 01/07/15 08:32, Ouyang Changchun wrote:

Check mq mode for VMDq RSS, handle it correctly instead of returning

an error; Also remove the limitation of per pool queue number has max

value of 1, because the per pool queue number could be 2 or 4 if it

is VMDq RSS mode;



The number of rxq specified in config will determine the mq mode for

VMDq RSS.



Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com><mailto:changchun.ouyang@intel.com>



changes in v5:

   - Fix '<' issue, it should be '<=' to test rxq number;

   - Extract a function to remove the embeded switch-case statement.



---

  lib/librte_ether/rte_ethdev.c | 50

++++++++++++++++++++++++++++++++++++++-----

  1 file changed, 45 insertions(+), 5 deletions(-)



diff --git a/lib/librte_ether/rte_ethdev.c

b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644

--- a/lib/librte_ether/rte_ethdev.c

+++ b/lib/librte_ether/rte_ethdev.c

@@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct

rte_eth_dev

*dev, uint16_t nb_queues)

  }

    static int

+rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)

+{

+    struct rte_eth_dev *dev = &rte_eth_devices[port_id];

+    switch (nb_rx_q) {

+    case 1:

+    case 2:

+        RTE_ETH_DEV_SRIOV(dev).active =

+            ETH_64_POOLS;

+        break;

+    case 4:

+        RTE_ETH_DEV_SRIOV(dev).active =

+            ETH_32_POOLS;

+        break;

+    default:

+        return -EINVAL;

+    }

+

+    RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = nb_rx_q;

+    RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx =

+        dev->pci_dev->max_vfs * nb_rx_q;

+

+    return 0;

+}

+

+static int

  rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,

uint16_t nb_tx_q,

                const struct rte_eth_conf *dev_conf)

  {

@@ -510,8 +535,7 @@ rte_eth_dev_check_mq_mode(uint8_t port_id,

uint16_t nb_rx_q, uint16_t nb_tx_q,

        if (RTE_ETH_DEV_SRIOV(dev).active != 0) {

          /* check multi-queue mode */

-        if ((dev_conf->rxmode.mq_mode == ETH_MQ_RX_RSS) ||

-            (dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB) ||

+        if ((dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB) ||

              (dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB_RSS) ||

              (dev_conf->txmode.mq_mode == ETH_MQ_TX_DCB)) {

              /* SRIOV only works in VMDq enable mode */ @@ -525,7

+549,6 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t

nb_rx_q, uint16_t nb_tx_q,

          }

            switch (dev_conf->rxmode.mq_mode) {

-        case ETH_MQ_RX_VMDQ_RSS:

          case ETH_MQ_RX_VMDQ_DCB:

          case ETH_MQ_RX_VMDQ_DCB_RSS:

              /* DCB/RSS VMDQ in SRIOV mode, not implement yet */ @@

-534,6 +557,25 @@ rte_eth_dev_check_mq_mode(uint8_t port_id,

uint16_t

nb_rx_q, uint16_t nb_tx_q,

                      "unsupported VMDQ mq_mode rx %u\n",

                      port_id, dev_conf->rxmode.mq_mode);

              return (-EINVAL);

+        case ETH_MQ_RX_RSS:

+            PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

+                    " SRIOV active, "

+                    "Rx mq mode is changed from:"

+                    "mq_mode %u into VMDQ mq_mode %u\n",

+                    port_id,

+                    dev_conf->rxmode.mq_mode,

+                    dev->data->dev_conf.rxmode.mq_mode);

+        case ETH_MQ_RX_VMDQ_RSS:

+            dev->data->dev_conf.rxmode.mq_mode =

ETH_MQ_RX_VMDQ_RSS;

+            if (nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool)

+                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,

nb_rx_q) != 0) {

+                    PMD_DEBUG_TRACE("ethdev port_id=%d"

+                        " SRIOV active, invalid queue"

+                        " number for VMDQ RSS\n",

+                        port_id);



Some nitpicking here: I'd add the allowed values descriptions to the

error message. Something like: "invalid queue number for VMDQ RSS.

Allowed values are 1, 2 or 4\n".



+                    return -EINVAL;

+                }

+            break;

          default: /* ETH_MQ_RX_VMDQ_ONLY or ETH_MQ_RX_NONE */

              /* if nothing mq mode configure, use default scheme */

              dev->data->dev_conf.rxmode.mq_mode =

ETH_MQ_RX_VMDQ_ONLY; @@ -553,8 +595,6 @@

rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,

uint16_t nb_tx_q,

          default: /* ETH_MQ_TX_VMDQ_ONLY or ETH_MQ_TX_NONE */

              /* if nothing mq mode configure, use default scheme */

              dev->data->dev_conf.txmode.mq_mode =

ETH_MQ_TX_VMDQ_ONLY;

-            if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)

-                RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;



I'm not sure u may just remove it. These lines originally belong to a

different flow. Are u sure u can remove them like that? What if the

mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized

to 4

or 8 in ixgbe_pf_host_init()?



I misread the patch - these lines belong to the txmode.mq_mode switch case.

I think it's ok to remove these really strange lines here. And when I look at it i

think for the similar reasons the similar lines should be removed in the Rx

case too: consider non-RSS case with MQ DCB Tx configuration.



I search code in this function, only one place has

" if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)

           RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"



The only place is default branch, which is for rx_none, or vmdq_only mode,

Here is a snippet of an rte_eth_dev_check_mq_mode() from the current master:

               switch (dev_conf->rxmode.mq_mode) {

               case ETH_MQ_RX_VMDQ_RSS:

               case ETH_MQ_RX_VMDQ_DCB:

               case ETH_MQ_RX_VMDQ_DCB_RSS:

                       /* DCB/RSS VMDQ in SRIOV mode, not implement yet */

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode rx %u\n",

                                      port_id, dev_conf->rxmode.mq_mode);

                       return (-EINVAL);

               default: /* ETH_MQ_RX_VMDQ_ONLY or ETH_MQ_RX_NONE */

                       /* if nothing mq mode configure, use default scheme */

                       dev->data->dev_conf.rxmode.mq_mode = ETH_MQ_RX_VMDQ_ONLY;

                       if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }



               switch (dev_conf->txmode.mq_mode) {

               case ETH_MQ_TX_VMDQ_DCB:

                       /* DCB VMDQ in SRIOV mode, not implement yet */

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode tx %u\n",

                                      port_id, dev_conf->txmode.mq_mode);

                       return (-EINVAL);

               default: /* ETH_MQ_TX_VMDQ_ONLY or ETH_MQ_TX_NONE */

                       /* if nothing mq mode configure, use default scheme */

                       dev->data->dev_conf.txmode.mq_mode = ETH_MQ_TX_VMDQ_ONLY;

                       if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)              <------ This is two. This is what your patch is removing

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }


Changchun: yes you are correct, what I mean in my last response is that only one place AFTER my removal, so there are 2 places before my removal.
no controversial here.




We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.

And why is that? Just because RSS was not enabled? And what if a user wants multiple Tx queues? Mode 1100b of MRQE for instance?

Changchun: I can explain why I need this change(remove the second place) here,
In the txmode, when txmode is ETH_MQ_TX_NONE, but the rx mode could either be ETH_MQ_RX_NONE or
ETH_MQ_RX_VMDQ_RSS, so we could not forcedly set nb_q_per_pool into 1 just hit the condition of txmode is ETH_MQ_TX_NONE,
Because we need consider it is combination of rx mode is ETH_MQ_RX_VMDQ_RSS, and tx mode is  ETH_MQ_TX_NONE,
In such a case, the queue number per pool could be 1, or 2, or 4.

In another hand, introducing ETH_MQ_TX_VMDQ_RSS for tx mode, seems very strange, because tx side has no rss feature.

thanks Changchun