On 01/12/15 05:41, Ouyang, Changchun wrote: > > *From:*Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > *Sent:* Friday, January 09, 2015 9:50 PM > *To:* Ouyang, Changchun; dev at 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 at cloudius-systems.com] > > Sent: Friday, January 9, 2015 2:49 AM > > To: Ouyang, Changchun;dev at dpdk.org <mailto:dev at 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 at > intel.com> <mailto:changchun.ouyang at 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. ;) > 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. > It's called ETH_MQ_TX_VMDQ_DCB in DPDK notation. ;) However I see that it's not yet supported. But *when* it's going to be supported the above code will turn to be bogus since u actually don't want to set the nb_q_per_pool to 1 neither if Rx mode is not MQ and nor if Tx mode is not MQ but only if them **both* *are not MQ. And this "if" is simply missing in the rte_eth_dev_check_mq_mode(). > > thanks Changchun > > >
