From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f179.google.com (mail-we0-f179.google.com [74.125.82.179]) by dpdk.org (Postfix) with ESMTP id B9CDD5424 for ; Thu, 22 Jan 2015 13:59:41 +0100 (CET) Received: by mail-we0-f179.google.com with SMTP id q59so1589573wes.10 for ; Thu, 22 Jan 2015 04:59:41 -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 :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=eBJ15AhcI7Pd0zBKQ3TSPdaXgd0KyBBr9XyLTnZoxrM=; b=SGgMyWawiZ/22mqRP/WOSCMvT5BN/Uv67fKUt6uLXfs1VTYLTg+B5RK0CxB0KycMVH xGQDdFRLOSve01ArduEiLjtplrkHTLgCwQW/uo+9uvSBVdqeOSjFSs8JNrRMe/EaO4a9 fzx4M6hJk2mS81GgIuAgKCJp95Rjo1gL0BcyufCX012eDNZS7bulBVz+sqsBlGQ0NHAB QqtoDnvurIC/ZooazJo1GwYWuDjj/paeCcM18kFHx20N/tnnbhsbLfxG4+ZvcJSKyLLn qEHZH+ShSDzKhaa399eKflKBUuO5t64abC+fBOaZOP5BdheWiddqvvkvmSoKADOLFKlk 2+2Q== X-Gm-Message-State: ALoCoQlTr24wLe2dv2leASZ2BSntqCzmaRen1N8/E9kROt9KUIOtQWfnJbRZCNG+7GcyIoqEj031 X-Received: by 10.180.210.174 with SMTP id mv14mr4879320wic.80.1421931581573; Thu, 22 Jan 2015 04:59:41 -0800 (PST) Received: from [10.0.0.165] (system.cloudius-systems.com. [84.94.198.183]) by mx.google.com with ESMTPSA id d6sm2979007wic.1.2015.01.22.04.59.39 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Jan 2015 04:59:41 -0800 (PST) Message-ID: <54C0F43A.2070804@cloudius-systems.com> Date: Thu, 22 Jan 2015 14:59:38 +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: "Wodkowski, PawelX" , "Ouyang, Changchun" , "dev@dpdk.org" References: <1420612355-6666-1-git-send-email-changchun.ouyang@intel.com> <1421042352-22399-1-git-send-email-changchun.ouyang@intel.com> <1421042352-22399-6-git-send-email-changchun.ouyang@intel.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS 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: Thu, 22 Jan 2015 12:59:42 -0000 On 01/21/15 10:44, Wodkowski, PawelX wrote: > >> -----Original Message----- >> From: Ouyang, Changchun >> Sent: Wednesday, January 21, 2015 3:44 AM >> To: Wodkowski, PawelX; dev@dpdk.org >> Cc: Ouyang, Changchun >> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS >> >> >> >>> -----Original Message----- >>> From: Wodkowski, PawelX >>> Sent: Tuesday, January 20, 2015 5:35 PM >>> To: Ouyang, Changchun; dev@dpdk.org >>> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang >>> Changchun >>>> Sent: Monday, January 12, 2015 6:59 AM >>>> To: dev@dpdk.org >>>> Subject: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS >>>> >>>> It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF >>> RSS. >>>> The psrtype will determine how many queues the received packets will >>>> distribute to, and the value of psrtype should depends on both facet: >>>> max VF rxq number which has been negotiated with PF, and the number of >>>> rxq specified in config on guest. >>>> >>>> Signed-off-by: Changchun Ouyang >>>> >>>> Changes in v6: >>>> - Raise an error for the case of ETH_16_POOLS in config vf rss, as the >>> previous >>>> logic have changed it into: ETH_32_POOLS. >>>> >>>> Changes in v4: >>>> - The number of rxq from config should be power of 2 and should not >>>> bigger than >>>> max VF rxq number(negotiated between guest and host). >>>> >>>> --- >>>> lib/librte_pmd_ixgbe/ixgbe_pf.c | 15 ++++++ >>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102 >>>> +++++++++++++++++++++++++++++++++----- >>>> 2 files changed, 105 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c >>>> b/lib/librte_pmd_ixgbe/ixgbe_pf.c index dbda9b5..93f6e43 100644 >>>> --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c >>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c >>>> @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev >>>> *eth_dev) >>>> IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw- >>>> mac.num_rar_entries), >>>> 0); >>>> IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw- >>>> mac.num_rar_entries), >>>> 0); >>>> >>>> + /* >>>> + * VF RSS can support at most 4 queues for each VF, even if >>>> + * 8 queues are available for each VF, it need refine to 4 >>>> + * queues here due to this limitation, otherwise no queue >>>> + * will receive any packet even RSS is enabled. >>>> + */ >>>> + if (eth_dev->data->dev_conf.rxmode.mq_mode == >>>> ETH_MQ_RX_VMDQ_RSS) { >>>> + if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) { >>>> + RTE_ETH_DEV_SRIOV(eth_dev).active = >>>> ETH_32_POOLS; >>>> + RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4; >>>> + RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx = >>>> + dev_num_vf(eth_dev) * 4; >>>> + } >>>> + } >>>> + >>> I did not looked before at your patches but I think you are messing with >>> things that should not be changed: >>> >>> Why you are changing those values. They are set up during >>> ixgbe_pf_host_init(). Limitation you are describing is only RSS related. If >>> there will be reconfiguration from ETH_MQ_RX_VMDQ_RSS to other mode >>> those value need to be re-evaluated. If you find this kind of limitation you >>> should handle it during RSS part configuration. Or if your way is the right way >>> you should explicitly make separate function that will re-evaluate those >>> parameters each time. >>> >>> Second issue with this code is that the nb_q_per_pool is changed from: >>> ixgbe_pf_host_configure() -> ixgbe_dev_start() -> rte_eth_dev_start() and >>> rte_eth_dev_check_vf_rss_rxq_num() -> rte_eth_dev_check_mq_mode() -> >>> rte_eth_dev_configure() >>> >>> Which one is the right one? If both, why they are calculated twice? >>> >>> I don't think that rte_eth_dev_data::sriov field should be changed at all - it >>> holds current SRIOV capabilities. >>> If this will change during runtime it no point to have this field at all and should >>> be some kind of "siov_get()" >>> function that will calculate and return those parameters dynamically. >>> >>> Please refer also to >>> >> >> .com> >>> for further issues. >>> >>> I think this patchset should not be applied. >> The better way should be either raise your comments before this patch is >> merged into mainline, or > Yes, I should but I trusted that Vlad review was covering this part. I'm new on the list and my experience with DPDK is about two months so, pls., don't judge me too harsh... ;) I tried to cover the obvious things and actually learned the code while reviewing. The things u say, Pavel(X?) make sense and I obviously missed that. But as Changchun mentioned there is nothing that can't be fixed with a followup patches... ;) > Does no matter > my, fault. > >> You send out a patch to fix it. >> I agree on part of what you said, the check is not necessary for vf rss in >> pf_host_configure because >> Check_mq_mode has already check the queue number, I will send out a patch to >> fix it by removing this check. >> >> On the other hand, I disagree with you on " rte_eth_dev_data::sriov field should >> be changed at all ", > This is my private opinion, but either way, recalculating those values or not, > it should be consistent and for feature development well documented when it is > evaluated. Changing something in function that's name is calculated > "rte_eth_dev_check_mq_mode()" is not so very obvious. > >> The reason we need refine those value, is that those value get in pf_init, which is >> called on dev probe stage, >> And those value are not accurate, they should vary according to mq mode, the >> mq mode could be determined only after >> Dev is configured. > If you think they are "not accurate" you should not calculate them because they are > invalid and make VF behavior undefined. VF can probe those values before you > make them "accurate" in port configuration phase. What then? It is a race condition > bug, and it definitely should be fixed in your next patch. > > You should also fix port reconfiguration bug as I mention before (for VFs > 0 testpmd > is unable to start port after commnad 'port config all rxq X', X > 1 after RSS VF > patches). > > Pawel > >