From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Zolotarov Subject: Re: [PATCH v5 4/6] ether: Check VMDq RSS mode Date: Tue, 13 Jan 2015 11:00:08 +0200 Message-ID: <54B4DE98.5030607@cloudius-systems.com> References: <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> <1420612355-6666-1-git-send-email-changchun.ouyang@intel.com> <1420612355-6666-5-git-send-email-changchun.ouyang@intel.com> <54AE4BA2.9040802@cloudius-systems.com> <54AED114.5070907@cloudius-systems.com> <54AFDC77.8040505@cloudius-systems.com> <54B3D30A.40108@cloudius-systems.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable To: "Ouyang, Changchun" , "dev-VfR2kkLFssw@public.gmane.org" Return-path: In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On 01/13/15 03:50, Ouyang, Changchun wrote: > > *From:*Vlad Zolotarov [mailto:vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org] > *Sent:* Monday, January 12, 2015 9:59 PM > *To:* Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.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-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org] > *Sent:* Friday, January 09, 2015 9:50 PM > *To:* Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org > *Subject:* Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode > > On 01/09/15 07:54, Ouyang, Changchun wrote: > > =20 > > =20 > > -----Original Message----- > > From: Vlad Zolotarov [mailto:vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org] > > Sent: Friday, January 9, 2015 2:49 AM > > To: Ouyang, Changchun;dev-VfR2kkLFssw@public.gmane.org > > Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RS= S mode > > =20 > > =20 > > On 01/08/15 11:19, Vlad Zolotarov wrote: > > =20 > > On 01/07/15 08:32, Ouyang Changchun wrote: > > Check mq mode for VMDq RSS, handle it correctly ins= tead of returning > > an error; Also remove the limitation of per pool qu= eue number has max > > value of 1, because the per pool queue number could= be 2 or 4 if it > > is VMDq RSS mode; > > =20 > > The number of rxq specified in config will determin= e the mq mode for > > VMDq RSS. > > =20 > > Signed-off-by: Changchun Ouyang > > =20 > > changes in v5: > > - Fix '<' issue, it should be '<=3D' to test rx= q number; > > - Extract a function to remove the embeded swit= ch-case statement. > > =20 > > --- > > lib/librte_ether/rte_ethdev.c | 50 > > ++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 45 insertions(+), 5 deletions(-) > > =20 > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363= e26 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(st= ruct > > 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 =3D &rte_eth_devices[p= ort_id]; > > + switch (nb_rx_q) { > > + case 1: > > + case 2: > > + RTE_ETH_DEV_SRIOV(dev).active =3D > > + ETH_64_POOLS; > > + break; > > + case 4: > > + RTE_ETH_DEV_SRIOV(dev).active =3D > > + ETH_32_POOLS; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool =3D nb_rx= _q; > > + RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx =3D > > + dev->pci_dev->max_vfs * nb_rx_q; > > + > > + return 0; > > +} > > + > > +static int > > rte_eth_dev_check_mq_mode(uint8_t port_id, uint1= 6_t nb_rx_q, > > uint16_t nb_tx_q, > > const struct rte_eth_conf *dev_con= f) > > { > > @@ -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 !=3D 0) = { > > /* check multi-queue mode */ > > - if ((dev_conf->rxmode.mq_mode =3D=3D ETH_M= Q_RX_RSS) || > > - (dev_conf->rxmode.mq_mode =3D=3D ETH_M= Q_RX_DCB) || > > + if ((dev_conf->rxmode.mq_mode =3D=3D ETH_M= Q_RX_DCB) || > > (dev_conf->rxmode.mq_mode =3D=3D ETH= _MQ_RX_DCB_RSS) || > > (dev_conf->txmode.mq_mode =3D=3D ETH= _MQ_TX_DCB)) { > > /* SRIOV only works in VMDq enable m= ode */ @@ -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 i= mplement 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=3D%" P= RIu8 > > + " 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 =3D > > ETH_MQ_RX_VMDQ_RSS; > > + if (nb_rx_q <=3D RTE_ETH_DEV_SRIOV(dev= ).nb_q_per_pool) > > + if (rte_eth_dev_check_vf_rss_rxq_n= um(port_id, > > nb_rx_q) !=3D 0) { > > + PMD_DEBUG_TRACE("ethdev port_i= d=3D%d" > > + " SRIOV active, invalid qu= eue" > > + " number for VMDQ RSS\n", > > + port_id); > > =20 > > Some nitpicking here: I'd add the allowed values descri= ptions to the > > error message. Something like: "invalid queue number fo= r VMDQ RSS. > > Allowed values are 1, 2 or 4\n". > > =20 > > + return -EINVAL; > > + } > > + break; > > default: /* ETH_MQ_RX_VMDQ_ONLY or ETH_M= Q_RX_NONE */ > > /* if nothing mq mode configure, use= default scheme */ > > dev->data->dev_conf.rxmode.mq_mode =3D > > 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_M= Q_TX_NONE */ > > /* if nothing mq mode configure, use= default scheme */ > > dev->data->dev_conf.txmode.mq_mode =3D > > ETH_MQ_TX_VMDQ_ONLY; > > - if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_po= ol > 1) > > - RTE_ETH_DEV_SRIOV(dev).nb_q_per_po= ol =3D 1; > > =20 > > I'm not sure u may just remove it. These lines original= ly 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 in= itialized > > to 4 > > or 8 in ixgbe_pf_host_init()? > > =20 > > I misread the patch - these lines belong to the txmode.mq_m= ode 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 r= emoved in the Rx > > case too: consider non-RSS case with MQ DCB Tx configuratio= n. > > =20 > > 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 =3D 1;" > > =20 > > 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 implemen= t yet */ > > PMD_DEBUG_TRACE("ethdev port_id=3D%" 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 defaul= t scheme */ > > dev->data->dev_conf.rxmode.mq_mode =3D 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_po= ol =3D 1;* > > break; > > } > > =20 > > switch (dev_conf->txmode.mq_mode) { > > case ETH_MQ_TX_VMDQ_DCB: > > /* DCB VMDQ in SRIOV mode, not implement ye= t */ > > PMD_DEBUG_TRACE("ethdev port_id=3D%" 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 defaul= t scheme */ > > dev->data->dev_conf.txmode.mq_mode =3D 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_poo= l =3D 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. > > > =20 > > 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=20 > exactly the same reasons u need to remove the "first place" too. ;) > > Changchun: then I will try to explain why I can=92t remove the first pl= ace 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=20 > 4 or 8 according to max vf num, > > (actually at that point, it has no knowledge of what is the rx and tx=20 > 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=20 > 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=20 > than 1, then it should refine to 1. > > So if I remove the first place, VMDQ_RSS case works well, but I break=20 > the case of RX_NONE. > > So I think we can=92t treat rx path and tx path in absolutely same way=20 > here, i.e. if you add it in the first place(rx path) then you need=20 > also add it in the second place(tx path) > > Vice versa, > > that=92s my understanding J > And now consider the case when rx_mode =3D=3D RSS_NONE (since user has=20 configured only a single Rx queue) and tx_mode =3D=3D TX_DCB (user has=20 configured 4 Tx queues and requested the above Tx mode). After your=20 patch the nb_q_per_pool will still be set to 1 while it should have=20 remained 4 because u want a pool to support 4 queues (MRQC.MRQE =3D=3D=20 1010b) but u will configure the PSRTYPE[n].RQPL for this pool to 0. > Thanks > > Changchun >