From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 443855A15 for ; Wed, 21 Jan 2015 09:44:12 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 21 Jan 2015 00:44:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,440,1418112000"; d="scan'208";a="654153718" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga001.fm.intel.com with ESMTP; 21 Jan 2015 00:44:10 -0800 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.28]) by IRSMSX107.ger.corp.intel.com ([169.254.10.75]) with mapi id 14.03.0195.001; Wed, 21 Jan 2015 08:44:08 +0000 From: "Wodkowski, PawelX" To: "Ouyang, Changchun" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS Thread-Index: AQHQLi0UAjB9Zu+3h0KBCsEOtdHcxJzIxiqwgAElsQCAAFk9UA== Date: Wed, 21 Jan 2015 08:44:08 +0000 Message-ID: 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: Accept-Language: pl-PL, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Wed, 21 Jan 2015 08:44:13 -0000 > -----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 >=20 >=20 >=20 > > -----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 o= f > > > 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 =3D=3D > > > ETH_MQ_RX_VMDQ_RSS) { > > > + if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool =3D=3D 8) { > > > + RTE_ETH_DEV_SRIOV(eth_dev).active =3D > > > ETH_32_POOLS; > > > + RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool =3D 4; > > > + RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =3D > > > + dev_num_vf(eth_dev) * 4; > > > + } > > > + } > > > + > > > > I did not looked before at your patches but I think you are messing wit= h > > 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 limitatio= n you > > should handle it during RSS part configuration. Or if your way is the r= ight way > > you should explicitly make separate function that will re-evaluate thos= e > > 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() a= nd > > 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 a= ll - it > > holds current SRIOV capabilities. > > If this will change during runtime it no point to have this field at al= l 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. >=20 > 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. Does n= o 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 i= n > 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. >=20 > 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 n= ot, it should be consistent and for feature development well documented when it= is=20 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 yo= u make them "accurate" in port configuration phase. What then? It is a race c= ondition 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=20 patches). Pawel