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: Monday, January 12, 2015 9:59 PM
To: Ouyang, Changchun; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/12/15 05:41, Ouyang, Changchun wrote:


From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Friday, January 09, 2015 9:50 PM
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/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,

   I understood why u needed it in the first place. I just say that for exactly the same reasons u need to remove the "first place" too. ;)


Changchun: then I will try to explain why I can't remove the first place :)
When the rx mode is ETH_MQ_RX_NONE and tx mode is ETH_MQ_TX_NONE,
The function ixgbe_pf_host_init still set the nb_q_per_pool into 2 or 4 or 8 according to max vf num,
(actually at that point, it has no knowledge of what is the rx and tx configuration value, so have to just set
an estimated (and not so accurate) value according to the max vf num)
then in the check_mq_mode function, need further refine this value according to a few factors:
sriov.active, and rxmode.mq_mode.
When it finds the rx mode is RX_NONE, and the nb_q_per_pool is larger than 1, then it should refine to 1.
So if I remove the first place, VMDQ_RSS case works well, but I break the case of RX_NONE.

So I think we can't treat rx path and tx path in absolutely same way here, i.e. if you add it in the first place(rx path) then you need also add it in the second place(tx path)
Vice versa,
that's my understanding :)

Thanks
Changchun