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. >>>>> Thanks >>>>> Changchun >>>>> >>>>>