> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > Sent: Wednesday, January 7, 2015 3:56 AM > To: Ouyang, Changchun; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode > > > On 01/06/15 03:56, Ouyang, Changchun wrote: > >> -----Original Message----- > >> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > >> Sent: Monday, January 5, 2015 6:10 PM > >> To: Ouyang, Changchun;dev at dpdk.org > >> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode > >> > >> > >> On 01/05/15 03:00, Ouyang, Changchun wrote: > >>>> -----Original Message----- > >>>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > >>>> Sent: Sunday, January 4, 2015 5:46 PM > >>>> To: Ouyang, Changchun;dev at dpdk.org > >>>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode > >>>> > >>>> > >>>> On 01/04/15 10:58, Ouyang, Changchun wrote: > >>>>>> -----Original Message----- > >>>>>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > >>>>>> Sent: Sunday, January 4, 2015 4:45 PM > >>>>>> To: Ouyang, Changchun;dev at dpdk.org > >>>>>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS > mode > >>>>>> > >>>>>> > >>>>>> On 01/04/15 09:18, 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 at intel.com> > >>>>>>> --- > >>>>>>> lib/librte_ether/rte_ethdev.c | 39 > >>>>>> ++++++++++++++++++++++++++++++++++----- > >>>>>>> 1 file changed, 34 insertions(+), 5 deletions(-) > >>>>>>> > >>>>>>> diff --git a/lib/librte_ether/rte_ethdev.c > >>>>>>> b/lib/librte_ether/rte_ethdev.c index 95f2ceb..59ff325 100644 > >>>>>>> --- a/lib/librte_ether/rte_ethdev.c > >>>>>>> +++ b/lib/librte_ether/rte_ethdev.c > >>>>>>> @@ -510,8 +510,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 +524,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 > >>>>>>> +532,39 @@ 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) { > >>>> Missed that before: shouldn't it be "<=" here? > >>> Agree with you, need <= here, I will fix it in v5 > >>> > >>>>>>> + 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: > >>>>>>> + > >> PMD_DEBUG_TRACE("ethdev > >>>>>> port_id=%d" > >>>>>>> + " SRIOV active, " > >>>>>>> + "queue number > >> invalid\n", > >>>>>>> + port_id); > >>>>>>> + 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; > >>>>>>> + } > >>>>>> Don't u need to return an error in the "else" here? > >>>>> Actually it has such a check after these code snippet, and it does > >>>>> return error for the else case, Because it is original logic, I > >>>>> don't change any > >>>> code around it, so it doesn't display here, you can check the codes. > >>>> > >>>> I see. The flow is a bit confusing since the switch-case above will > >>>> end up executing a "default" clause which will set > >>>> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 and then the error > >> message > >>>> in the check u are referring will be a bit confusing. > >>> ' set RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 ' is original code, > >> which is for vmdq only case, or single queue case. > >>> It is in default clause, and not in VMDQ_RSS clause. > >>> I think my new code is ok here. > >> The original code is ok and your current code will work. The only > >> problem with your new code is that in case on an error like I've > >> described above the error message will be confusing. > > Then what's your suggestion for the better log message? I can consider > refine it if you have better one. > > Just like I've suggested before - u may break with appropriate error message > right when u see the problem (in a "else" clause). This way the code will be > both more readable and more robust and won't break if anybody decides to > change the not-RSS-specific logic u r relying on.
Well, it couldn't be done so easily, I think, the test condition is: if (nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool), so the else clause is the case of nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool, its functionality is comparing nb_rx_q and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool, but the switch case will further confine nb_rx_q to 1 or 2 or 4 on the condition of it passes the above test, and also there are codes refine the RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool etc. just changing the return into break, will break the logic, e.g. when nb_rx_q is 8, and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool is 8, the test pass, and go into default branch, it just print some message and break, continue refining(but nothing changed this time), then check valid queue number a few lines below, this time it fail the test, because nb_rx_q == rather than > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool , so it doesn't print err mesge and don't return the -EINVAL. Then the behavior is not expected.
