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

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

> -----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
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 ",
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.

> 
> >  	/* set VMDq map to default PF pool */
> >  	hw->mac.ops.set_vmdq(hw, 0,
> > RTE_ETH_DEV_SRIOV(eth_dev).def_vmdq_idx);
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > index f69abda..20627df 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > @@ -3327,6 +3327,67 @@ ixgbe_alloc_rx_queue_mbufs(struct
> igb_rx_queue
> > *rxq)
> >  }
> >
> >  static int
> > +ixgbe_config_vf_rss(struct rte_eth_dev *dev) {
> > +	struct ixgbe_hw *hw;
> > +	uint32_t mrqc;
> > +
> > +	ixgbe_rss_configure(dev);
> > +
> > +	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +
> > +	/* MRQC: enable VF RSS */
> > +	mrqc = IXGBE_READ_REG(hw, IXGBE_MRQC);
> > +	mrqc &= ~IXGBE_MRQC_MRQE_MASK;
> > +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> > +	case ETH_64_POOLS:
> > +		mrqc |= IXGBE_MRQC_VMDQRSS64EN;
> > +		break;
> > +
> > +	case ETH_32_POOLS:
> > +		mrqc |= IXGBE_MRQC_VMDQRSS32EN;
> > +		break;
> > +
> > +	default:
> > +		PMD_INIT_LOG(ERR, "Invalid pool number in IOV mode with
> > VMDQ RSS");
> > +		return -EINVAL;
> > +	}
> > +
> > +	IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +ixgbe_config_vf_default(struct rte_eth_dev *dev) {
> > +	struct ixgbe_hw *hw =
> > +		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +
> > +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> > +	case ETH_64_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQEN);
> > +		break;
> > +
> > +	case ETH_32_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQRT4TCEN);
> > +		break;
> > +
> > +	case ETH_16_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQRT8TCEN);
> > +		break;
> > +	default:
> > +		PMD_INIT_LOG(ERR,
> > +			"invalid pool number in IOV mode");
> > +		break;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int
> >  ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)  {
> >  	struct ixgbe_hw *hw =
> > @@ -3358,24 +3419,25 @@ ixgbe_dev_mq_rx_configure(struct
> rte_eth_dev
> > *dev)
> >  			default: ixgbe_rss_disable(dev);
> >  		}
> >  	} else {
> > -		switch (RTE_ETH_DEV_SRIOV(dev).active) {
> >  		/*
> >  		 * SRIOV active scheme
> > -		 * FIXME if support DCB/RSS together with VMDq & SRIOV
> > +		 * Support RSS together with VMDq & SRIOV
> >  		 */
> > -		case ETH_64_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQEN);
> > -			break;
> > -
> > -		case ETH_32_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQRT4TCEN);
> > +		switch (dev->data->dev_conf.rxmode.mq_mode) {
> > +		case ETH_MQ_RX_RSS:
> > +		case ETH_MQ_RX_VMDQ_RSS:
> > +			ixgbe_config_vf_rss(dev);
> >  			break;
> >
> > -		case ETH_16_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQRT8TCEN);
> > -			break;
> > +		/* FIXME if support DCB/RSS together with VMDq & SRIOV */
> > +		case ETH_MQ_RX_VMDQ_DCB:
> > +		case ETH_MQ_RX_VMDQ_DCB_RSS:
> > +			PMD_INIT_LOG(ERR,
> > +				"Could not support DCB with VMDq &
> SRIOV");
> > +			return -1;
> >  		default:
> > -			PMD_INIT_LOG(ERR, "invalid pool number in IOV
> > mode");
> > +			ixgbe_config_vf_default(dev);
> > +			break;
> >  		}
> >  	}
> >
> > @@ -3993,6 +4055,19 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
> >  	PMD_INIT_FUNC_TRACE();
> >  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> > +	if (rte_is_power_of_2(dev->data->nb_rx_queues) == 0) {
> > +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> > +			"it should be power of 2");
> > +		return -1;
> > +	}
> > +
> > +	if (dev->data->nb_rx_queues > hw->mac.max_rx_queues) {
> > +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> > +			"it should be equal to or less than %d",
> > +			hw->mac.max_rx_queues);
> > +		return -1;
> > +	}
> > +
> >  	/*
> >  	 * When the VF driver issues a IXGBE_VF_RESET request, the PF
> driver
> >  	 * disables the VF receipt of packets if the PF MTU is > 1500.
> > @@ -4094,6 +4169,9 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
> >  			IXGBE_PSRTYPE_IPV6HDR;
> >  #endif
> >
> > +	/* Set RQPL for VF RSS according to max Rx queue */
> > +	psrtype |= (dev->data->nb_rx_queues >> 1) <<
> > +		IXGBE_PSRTYPE_RQPL_SHIFT;
> >  	IXGBE_WRITE_REG(hw, IXGBE_VFPSRTYPE, psrtype);
> >
> >  	if (dev->data->dev_conf.rxmode.enable_scatter) {
> > --
> > 1.8.4.2