From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by dpdk.org (Postfix) with ESMTP id CC9F45A70 for ; Tue, 13 Jan 2015 10:00:11 +0100 (CET) Received: by mail-wi0-f172.google.com with SMTP id n3so19649769wiv.5 for ; Tue, 13 Jan 2015 01:00:11 -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=J7MtzitMt1UIk0XMyH4V2HkhgAQ2iO6cDSH6RF9b+TA=; b=FucpVDT66srgQgnWYzNRzMegJY90Gz6+q2R7D6k51ol+GbBps/CjUBxpAqAsMmVhQB zZGQ7729pdr7RfWp/kKh4ITlNB6GX6wsalaurBI/5TUMG0NFvQJZ0wDPkXtWVM6Fsqwp 5+OkHTspjveRFoLsvqYZJUoa0cNWnz5NEWmBubZf81TTl8/MWk8xPdTo4IuRCr+orxe0 rjYPWTjt5Ff6MW5BFfNLz9SEB70nmHaBWaUcLnoglLinx3fDqhTa6uUiXMcwJxU5GNsd ZVVlwNGZuKGARDvo0FVKQ0yDspTDYR7L1wbJg53SxhVI8+CC+7bA1fB2erW57TSTnDq+ wNlg== X-Gm-Message-State: ALoCoQlOsH1oZe0hCTnPTiipNJj3LOspJEBOHm0zLhKX+mOmO6fDM9FdfMdJpQGApT6qJ980GCYk X-Received: by 10.194.109.9 with SMTP id ho9mr17962605wjb.29.1421139611387; Tue, 13 Jan 2015 01:00:11 -0800 (PST) Received: from [10.0.0.4] ([109.66.137.113]) by mx.google.com with ESMTPSA id ej10sm13438040wib.2.2015.01.13.01.00.09 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 13 Jan 2015 01:00:10 -0800 (PST) Message-ID: <54B4DE98.5030607@cloudius-systems.com> Date: Tue, 13 Jan 2015 11:00:08 +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> <54B3D30A.40108@cloudius-systems.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit 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: Tue, 13 Jan 2015 09:00:13 -0000 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 > *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. ;) > > 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 >