On 2/1/2023 3:22 PM, Jerin Jacob wrote: > On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit <ferruh.yi...@amd.com> wrote: >> >> On 2/1/2023 1:48 PM, Jerin Jacob wrote: >>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit <ferruh.yi...@amd.com> wrote: >>>> >>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote: >>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon <tho...@monjalon.net> >>>>> wrote: >>>>>> >>>>>> 01/02/2023 11:58, Andrew Rybchenko: >>>>>>> On 2/1/23 13:48, Jerin Jacob wrote: >>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko >>>>>>>> <andrew.rybche...@oktetlabs.ru> wrote: >>>>>>>>> Frankly speaking I don't understand why default value is so >>>>>>>>> important if we have a way to change it. Reasons should be >>>>>>>>> really strong to change existing defaults. >>>>>>>> >>>>>>>> The only reason is, typically testpmd will be used performance >>>>>>>> benchmarking as an industry standard. It is difficult to tell/educate >>>>>>>> the QA or customers >>>>>>>> that, "BTW if you need to get better performance add more flag to >>>>>>>> testpmd command line". >>>>>> >>>>>> I disagree. >>>>>> When you do performance benchmark, you tune settings accordingly. >>>>> >>>>> IMO, We tune the system resources like queue depth not the disabling >>>>> features for raw performance. >>>>> queue depth etc people know to tune so it is obvious. What is not >>>>> obvious is, testpmd only >>>>> negotiated some features by default.I am not using that feature, hence >>>>> I need to explicitly >>>>> disable it. >>>>> >>>> >>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I >>>> believe that is the case for almost all applications since API is a >>>> relatively new one, PMD default behavior should be to enable Rx metadata >>>> flow rules, in case user requests them later. >>>> >>>> So, enabling all in application is same with not calling the API at all. >>>> >>>> In this perspective, disabling Rx metadata is additional >>>> optimization/tuning that application can do if it is sure that Rx >>>> metadata flow rules won't be used at all. >>>> And API is more meaningful when it is used to disable Rx metadata. >>>> >>>> I think it is reasonable to enable all Rx metadata by default in testpmd >>>> with a capability to disable it when wanted. >>>> >>>> OR >>>> >>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in >>>> testpmd, it is only called when it is requested explicitly from user, >>>> enable or disable. >>> >>> Second option looks good to me. >>> When >>> 1) user request for action which is needed negotiate(), >>> AND >>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP >>> then, testpmd print a warning that need to enable >>> rte_eth_rx_metadata_negotiate(). >>> >> >> We are not suggesting same thing. >> >> What you described above assumes PMD disabled Rx metadata flow rule >> support by default, and it needs to be enabled explicitly by >> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for >> functionality. >> >> As far as I understand PMD wants to disable this flow rule by default >> because of performance concerns. But this creates inconsistency between >> PMDs, because rest of them will enable this flow rule by default (if it >> is supported) and be ready to use it when proper flow rule created. >> >> With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()' >> to enable Rx metadata flow rules, some won't. This can be confusing for >> applications that *some* PMDs require double enabling with specific API >> call. >> >> >> Instead what I was trying to suggest is reverse, >> all PMDs enable the Rx metadata flow rule by default, and don't require >> double enabling. >> But if application knows that it won't use Rx metadata flow rule, it can >> disable it to optimize the performance. >> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and >> for testpmd context it can be called via a command on demand by user for >> optimization purpose. > > This won't solve concern I have outlined earlier[1]. >
Yes, it won't. > I think, The part of the problem there is no enough adaption of > rte_eth_rx_metadata_negotiate(), > > The view is total different from PMD maintainer PoV vs testpmd application > PoV. > Agree, and I assume it is different for user application too, which may prioritize consistency and portability. Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I think it is confusing. > Just to avoid back and forth. We will call off this patch and remove > rte_eth_rx_metadata_negotiate() > PMD callback from cnxk driver. Keep it as old behavior, so we don't need to > care > about rte_eth_rx_metadata_negotiate(). > When you remove 'rx_metadata_negotiate' callback, what will be the PMD behavior? I assume PMD will do the required preparations as if all Rx metadata is enabled. And what is the performance impact, is removing callback improve the performance? > [1] > The only reason is, typically testpmd will be used performance > benchmarking as an industry standard. It is difficult to tell/educate > the QA or customers > that, "BTW if you need to get better performance add more flag to > testpmd command line". > To make that worst, only some PMD needs to give the additional > parameter to get better number. > And also, testpmd usage will be treated as application modeling. > >> >> >> >> >>> >>>>> >>>>>> >>>>>>>> To make that worst, only some PMD needs to give the additional >>>>>>>> parameter to get better number. >>>>>>>> And also, testpmd usage will be treated as application modeling. >>>>>>>> >>>>>>>> Since this feature only used on sfc and cnxk driver, What is the >>>>>>>> situation with sfc driver? >>>>>>>> Keeping it as negotiated and not use the feature, will impact the per >>>>>>>> core performance of sfc or >>>>>>>> is it just PCI bandwidth thing which really dont show any difference >>>>>>>> in testpmd? >>>>>>> >>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So, >>>>>>> it is better to negotiate nothing by default. But it is always >>>>>>> painful to change defaults. You need to explain that now you >>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads. >>>>>>> Yes, it will be required on sfc and cnxk only. >>>>>>> As an sfc maintainer I don't mind to change testpmd defaults. >>>>>> >>>>>> If we change testpmd defaults to "do nothing", >>>>>> then we should disable MBUF_FAST_FREE as well. >>>>> >>>>> if you see MBUF_FAST_FREE, it does nothing. Actually, >>>>> !MBUF_FAST_FREE is doing more work. >>>>> >>>>> >>>>>> >>>>>> >>>> >>
