On 01/13/15 03:50, Ouyang, Changchun wrote: > > *From:*Vlad Zolotarov [mailto:vladz at cloudius-systems.com] > *Sent:* Monday, January 12, 2015 9:59 PM > *To:* Ouyang, Changchun; dev at 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 at cloudius-systems.com] > *Sent:* Friday, January 09, 2015 9:50 PM > *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/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. ;) > > Changchun: then I will try to explain why I can?t remove the first place J > > 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 J >
And now consider the case when rx_mode == RSS_NONE (since user has configured only a single Rx queue) and tx_mode == TX_DCB (user has configured 4 Tx queues and requested the above Tx mode). After your patch the nb_q_per_pool will still be set to 1 while it should have remained 4 because u want a pool to support 4 queues (MRQC.MRQE == 1010b) but u will configure the PSRTYPE[n].RQPL for this pool to 0. > Thanks > > Changchun >
