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