Message ID | 1420355937-18484-5-git-send-email-changchun.ouyang@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 369EC5A64; Sun, 4 Jan 2015 08:19:16 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 229E8594E for <dev@dpdk.org>; Sun, 4 Jan 2015 08:19:14 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 03 Jan 2015 23:16:55 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,694,1413270000"; d="scan'208";a="646040640" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by fmsmga001.fm.intel.com with ESMTP; 03 Jan 2015 23:19:14 -0800 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t047JCC4032100; Sun, 4 Jan 2015 15:19:12 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t047J9pr018546; Sun, 4 Jan 2015 15:19:11 +0800 Received: (from couyang@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t047J80K018542; Sun, 4 Jan 2015 15:19:08 +0800 From: Ouyang Changchun <changchun.ouyang@intel.com> To: dev@dpdk.org Date: Sun, 4 Jan 2015 15:18:55 +0800 Message-Id: <1420355937-18484-5-git-send-email-changchun.ouyang@intel.com> X-Mailer: git-send-email 1.7.12.2 In-Reply-To: <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> References: <1419398584-19520-1-git-send-email-changchun.ouyang@intel.com> <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> Subject: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Ouyang Changchun
Jan. 4, 2015, 7:18 a.m. UTC
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@intel.com>
---
lib/librte_ether/rte_ethdev.c | 39 ++++++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
Comments
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@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) { > + 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? > + 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 +584,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; > break; > } >
> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Sunday, January 4, 2015 4:45 PM > To: Ouyang, Changchun; dev@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@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) { > > + 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. Thanks Changchun
On 01/04/15 10:58, Ouyang, Changchun wrote: >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Sunday, January 4, 2015 4:45 PM >> To: Ouyang, Changchun; dev@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@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? >>> + 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. > > Thanks > Changchun > >
> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Sunday, January 4, 2015 5:46 PM > To: Ouyang, Changchun; dev@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@cloudius-systems.com] > >> Sent: Sunday, January 4, 2015 4:45 PM > >> To: Ouyang, Changchun; dev@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@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. > > > > Thanks > > Changchun > > > >
On 01/05/15 03:00, Ouyang, Changchun wrote: > >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Sunday, January 4, 2015 5:46 PM >> To: Ouyang, Changchun; dev@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@cloudius-systems.com] >>>> Sent: Sunday, January 4, 2015 4:45 PM >>>> To: Ouyang, Changchun; dev@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@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. > >>> Thanks >>> Changchun >>> >>>
> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Monday, January 5, 2015 6:10 PM > To: Ouyang, Changchun; dev@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@cloudius-systems.com] > >> Sent: Sunday, January 4, 2015 5:46 PM > >> To: Ouyang, Changchun; dev@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@cloudius-systems.com] > >>>> Sent: Sunday, January 4, 2015 4:45 PM > >>>> To: Ouyang, Changchun; dev@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@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 > >>> > >>>
On 01/06/15 03:56, Ouyang, Changchun wrote: >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Monday, January 5, 2015 6:10 PM >> To: Ouyang, Changchun;dev@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@cloudius-systems.com] >>>> Sent: Sunday, January 4, 2015 5:46 PM >>>> To: Ouyang, Changchun;dev@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@cloudius-systems.com] >>>>>> Sent: Sunday, January 4, 2015 4:45 PM >>>>>> To: Ouyang, Changchun;dev@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@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 >>>>> >>>>>
> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Wednesday, January 7, 2015 3:56 AM > To: Ouyang, Changchun; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode > > > On 01/06/15 03:56, Ouyang, Changchun wrote: > >> -----Original Message----- > >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > >> Sent: Monday, January 5, 2015 6:10 PM > >> To: Ouyang, Changchun;dev@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@cloudius-systems.com] > >>>> Sent: Sunday, January 4, 2015 5:46 PM > >>>> To: Ouyang, Changchun;dev@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@cloudius-systems.com] > >>>>>> Sent: Sunday, January 4, 2015 4:45 PM > >>>>>> To: Ouyang, Changchun;dev@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@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. Well, it couldn't be done so easily, I think, the test condition is: if (nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool), so the else clause is the case of nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool, its functionality is comparing nb_rx_q and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool, but the switch case will further confine nb_rx_q to 1 or 2 or 4 on the condition of it passes the above test, and also there are codes refine the RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool etc. just changing the return into break, will break the logic, e.g. when nb_rx_q is 8, and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool is 8, the test pass, and go into default branch, it just print some message and break, continue refining(but nothing changed this time), then check valid queue number a few lines below, this time it fail the test, because nb_rx_q == rather than > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool , so it doesn't print err mesge and don't return the -EINVAL. Then the behavior is not expected. From other hand, The reason why I have not the else branch for the test nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool, It is because there is same check below itself, and just don't want the duplicated check for the same thing > >>>>> Thanks > >>>>> Changchun > >>>>> > >>>>>
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) { + 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; + } + 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 +584,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; break; }