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

[Vlad Zolotarov <vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org>]

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-VfR2kkLFssw@public.gmane.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-VfR2kkLFssw@public.gmane.org
>>> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Ouyang
>>> Changchun
>>>> Sent: Monday, January 12, 2015 6:59 AM
>>>> To: dev-VfR2kkLFssw@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>
>>>> 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-kPTMFJFq+rFP9JyJpTNKArfspsVTdybX@public.gmane.org
>>> .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
>
>