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