| Message ID | 1420612355-6666-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 5AB665A75; Wed, 7 Jan 2015 07:32:59 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 284205A6A for <dev@dpdk.org>; Wed, 7 Jan 2015 07:32:54 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 06 Jan 2015 22:32:52 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,713,1413270000"; d="scan'208";a="657950942" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by fmsmga002.fm.intel.com with ESMTP; 06 Jan 2015 22:32:52 -0800 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t076WojY001198; Wed, 7 Jan 2015 14:32:50 +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 t076Wl3a006727; Wed, 7 Jan 2015 14:32:49 +0800 Received: (from couyang@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t076Wl7r006723; Wed, 7 Jan 2015 14:32:47 +0800 From: Ouyang Changchun <changchun.ouyang@intel.com> To: dev@dpdk.org Date: Wed, 7 Jan 2015 14:32:33 +0800 Message-Id: <1420612355-6666-5-git-send-email-changchun.ouyang@intel.com> X-Mailer: git-send-email 1.7.12.2 In-Reply-To: <1420612355-6666-1-git-send-email-changchun.ouyang@intel.com> References: <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> <1420612355-6666-1-git-send-email-changchun.ouyang@intel.com> Subject: [dpdk-dev] [PATCH v5 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. 7, 2015, 6:32 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>
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(-)
Comments
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@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()? > break; > } >
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@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. > >> break; >> } >
> -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Friday, January 9, 2015 2:49 AM > To: Ouyang, Changchun; dev@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@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, We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.
On 01/09/15 07:54, Ouyang, Changchun wrote: > >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Friday, January 9, 2015 2:49 AM >> To: Ouyang, Changchun; dev@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@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; } > 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?
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] Sent: Friday, January 09, 2015 9:50 PM To: Ouyang, Changchun; dev@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@cloudius-systems.com] Sent: Friday, January 9, 2015 2:49 AM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@intel.com><mailto:changchun.ouyang@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, 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. thanks Changchun
On 01/12/15 05:41, Ouyang, Changchun wrote: > > *From:*Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > *Sent:* Friday, January 09, 2015 9:50 PM > *To:* Ouyang, Changchun; dev@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@cloudius-systems.com] > > Sent: Friday, January 9, 2015 2:49 AM > > To: Ouyang, Changchun;dev@dpdk.org <mailto:dev@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@intel.com> <mailto:changchun.ouyang@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 > > >
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] Sent: Monday, January 12, 2015 9:59 PM To: Ouyang, Changchun; dev@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@cloudius-systems.com] Sent: Friday, January 09, 2015 9:50 PM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@cloudius-systems.com] Sent: Friday, January 9, 2015 2:49 AM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@intel.com><mailto:changchun.ouyang@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 :) 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 :) Thanks Changchun
On 01/13/15 03:50, Ouyang, Changchun wrote: > > *From:*Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > *Sent:* Monday, January 12, 2015 9:59 PM > *To:* Ouyang, Changchun; dev@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@cloudius-systems.com] > *Sent:* Friday, January 09, 2015 9:50 PM > *To:* Ouyang, Changchun; dev@dpdk.org <mailto:dev@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@cloudius-systems.com] > > Sent: Friday, January 9, 2015 2:49 AM > > To: Ouyang, Changchun;dev@dpdk.org <mailto:dev@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@intel.com> <mailto:changchun.ouyang@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 >
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] Sent: Tuesday, January 13, 2015 5:00 PM To: Ouyang, Changchun; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode On 01/13/15 03:50, Ouyang, Changchun wrote: From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] Sent: Monday, January 12, 2015 9:59 PM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@cloudius-systems.com] Sent: Friday, January 09, 2015 9:50 PM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@cloudius-systems.com] Sent: Friday, January 9, 2015 2:49 AM To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@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@intel.com><mailto:changchun.ouyang@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 :) 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 :) 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. [Changchun] As currently vmdq dcb is not supported yet, so it don't consider that case, as vf rss(vmdq rss) concerned, this patch is ok, I think you also agree that, am I right? Go back to your question, considering your case, with vmdq dcb, you are right, So as we can see Jastrzebski, MichalX K michalx.k.jastrzebski@intel.com<mailto:michalx.k.jastrzebski@intel.com> resolve this issue in his "add dcb for vf for ixgbe" by split nb_q_per_pool into nb_rx_q_per_pool, and nb_tx_q_per_pool, I thinks that's good way to do it. So my opinion is we can discuss this in "dcb for vf for ixgbe", because the question is now switch to dcb for vf, not rss for vf itself, How do you think of it? Changchun
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); + 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; break; }