Re: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

> -----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 <changchun.ouyang@intel.com>
> > >
> > > 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
> >
> <F6F2A6264E145F47A18AB6DF8E87425D12B89B02@IRSMSX102.ger.corp.intel
> > .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. 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