From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f41.google.com (mail-wg0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 14D175AA2 for ; Mon, 12 Jan 2015 14:58:42 +0100 (CET) Received: by mail-wg0-f41.google.com with SMTP id l18so19446288wgh.0 for ; Mon, 12 Jan 2015 05:58:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type; bh=zgNxeKJcKrAjPnSUvuN3JO1ZkMpwLwI1Au6W6D09ikQ=; b=XIbOjLLLOQWqO3tG82izFJ06MI3N9nTCmdF6zheN6mCePFqRNbIUup8WeCEyEaFck2 UZ/NSe602Xra9RsVOTzD0lsqdH9MlQvAMsPDcBiAH0YrlrVVR2qWUzFaMqUAGnkTx+5b GJ9fRmU1nRFWmrqqQMGmR2c/NSJJlE5RxHPt4XbpFL5YQQ8XDJWcebNBJ2jo/h5vTKVC fbRvo0EOJWZvZB9jrIvpaReW/q5/ETpx8Qr1d3AO1EU9R2DDb2EsvvzGxSw82bgJw9wl QVrxp8bTGlE3SQeMAoYXp9FvDZJ3/+untajEL97b7SxqQ03ILfjVb73kMj73y2Pfky9c gp4w== X-Gm-Message-State: ALoCoQkuOtz+sN5VpNsTHG6ZS6PBi7sQgmn+Na+NyF1cHqbfJMBfS8WxUBkdFcIaKq2CaH7aRn7K X-Received: by 10.194.81.1 with SMTP id v1mr29687303wjx.50.1421071121842; Mon, 12 Jan 2015 05:58:41 -0800 (PST) Received: from [10.0.0.165] (system.cloudius-systems.com. [84.94.198.183]) by mx.google.com with ESMTPSA id mo12sm21726953wjc.35.2015.01.12.05.58.38 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Jan 2015 05:58:40 -0800 (PST) Message-ID: <54B3D30A.40108@cloudius-systems.com> Date: Mon, 12 Jan 2015 15:58:34 +0200 From: Vlad Zolotarov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: "Ouyang, Changchun" , "dev@dpdk.org" References: <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> <1420612355-6666-1-git-send-email-changchun.ouyang@intel.com> <1420612355-6666-5-git-send-email-changchun.ouyang@intel.com> <54AE4BA2.9040802@cloudius-systems.com> <54AED114.5070907@cloudius-systems.com> <54AFDC77.8040505@cloudius-systems.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Jan 2015 13:58:42 -0000 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 > > 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 > > > > 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 > > >