On 10/24/2019 10:47 AM, Ferruh Yigit wrote: > On 10/22/2019 4:20 PM, Pavan Nikhilesh Bhagavatula wrote: >>> -----Original Message----- >>> From: Ferruh Yigit <ferruh.yi...@intel.com> >>> Sent: Tuesday, October 22, 2019 7:51 PM >>> To: Andrew Rybchenko <arybche...@solarflare.com>; Pavan Nikhilesh >>> Bhagavatula <pbhagavat...@marvell.com>; Jerin Jacob Kollanukkaran >>> <jer...@marvell.com> >>> Cc: dev@dpdk.org; Adrien Mazarguil <adrien.mazarg...@6wind.com>; >>> Thomas Monjalon <tho...@monjalon.net>; Xiaolong Ye >>> <xiaolong...@intel.com>; Bruce Richardson >>> <bruce.richard...@intel.com> >>> Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx >>> offload flags >>> On 10/22/2019 11:16 AM, Andrew Rybchenko wrote: >>>> Hi, >>>> >>>> @Pavan, see question below. >>>> >>>> On 10/21/19 6:34 PM, Ferruh Yigit wrote: >>>>> On 10/21/2019 4:19 PM, Pavan Nikhilesh Bhagavatula wrote: >>>>>> Hi Ferruh, >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Ferruh Yigit <ferruh.yi...@intel.com> >>>>>>> Sent: Monday, October 21, 2019 8:37 PM >>>>>>> To: Andrew Rybchenko <arybche...@solarflare.com>; Pavan >>> Nikhilesh >>>>>>> Bhagavatula <pbhagavat...@marvell.com>; Jerin Jacob >>> Kollanukkaran >>>>>>> <jer...@marvell.com> >>>>>>> Cc: dev@dpdk.org; Adrien Mazarguil >>> <adrien.mazarg...@6wind.com>; >>>>>>> Thomas Monjalon <tho...@monjalon.net>; Xiaolong Ye >>>>>>> <xiaolong...@intel.com>; Bruce Richardson >>>>>>> <bruce.richard...@intel.com> >>>>>>> Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx >>>>>>> offload flags >>>>>>> On 10/18/2019 11:31 AM, Andrew Rybchenko wrote: >>>>>>>> On 10/18/19 12:42 PM, Ferruh Yigit wrote: >>>>>>>>> On 10/18/2019 8:32 AM, Andrew Rybchenko wrote: >>>>>>>>>> Hi Ferruh, >>>>>>>>>> >>>>>>>>>> since I've reviewed I'll reply as I understand it. >>>>>>>>>> >>>>>>>>>> On 10/17/19 8:43 PM, Ferruh Yigit wrote: >>>>>>>>>>> On 10/17/2019 1:02 PM, pbhagavat...@marvell.com wrote: >>>>>>>>>>>> From: Pavan Nikhilesh <pbhagavat...@marvell.com> >>>>>>>>>>>> >>>>>>>>>>>> Add new Rx offload flags `DEV_RX_OFFLOAD_RSS_HASH` >>> and >>>>>>>>>>>> `DEV_RX_OFFLOAD_FLOW_MARK`. These flags can be >>> used to >>>>>>>>>>>> enable/disable PMD writes to rte_mbuf fields `hash.rss` >>> and >>>>>>> `hash.fdir.hi` >>>>>>>>>>>> and also `ol_flags:PKT_RX_RSS` and >>> `ol_flags:PKT_RX_FDIR`. >>>>>>>>>>> Hi Pavan, >>>>>>>>>>> >>>>>>>>>>> Initially sorry for involving late, >>>>>>>>>>> >>>>>>>>>>> When we expose an interface to the applications, they will >>> expect >>>>>>> those will be >>>>>>>>>>> respected by underlying PMDs. >>>>>>>>>>> As far as I can see drivers are updated to report new added >>> Rx >>>>>>> offload flags as >>>>>>>>>>> supported capabilities but drivers are not using those flags at >>> all, >>>>>>> so >>>>>>>>>>> application providing that flag won't really enable/disable >>>>>>> anything, I think >>>>>>>>>>> this is a problem and it is wrong to lie even for the PMDs J >>>>>>>>>> It is required to let applications know that the offload is >>> supported. >>>>>>>>>> There are a number of cases when an offload cannot be >>> disabled, >>>>>>>>>> but it does not mean that the offload must not be advertised. >>>>>>>>> Can't disable is something else, although I believe that is rare >>> case, in >>>>>>> this >>>>>>>>> case driver can enable/disable the RSS and representing this as >>> an >>>>>>> offload >>>>>>>>> capability. >>>>>>>> It is not enabling/disabling the RSS. It is enabling/disabling RSS >>> hash >>>>>>>> delivery >>>>>>>> together with an mbuf. >>>>>>> >>>>>>> Got it, it is related to the RSS hash delivery. >>>>>>> >>>>>>>>> But when user want to configure this offload by setting or >>> unsetting >>>>>>> in offload >>>>>>>>> config, driver just ignores it. >>>>>>>> When application enables offload, it says that it needs it and >>> going to >>>>>>> use >>>>>>>> (required). When the offload is not enabled, application simply >>> don't >>>>>>> care. >>>>>>>> So, if the information is still provided it does not harm. >>>>>>> >>>>>>> Not sure if there is no harm, a config option not respected by >>>>>>> underlying PMDs >>>>>>> silently is a problem I think. >>>> >>>> Some time ago the idea of offloads which cannot be disabled >>>> by PMD was discussed and, if I remember decision correctly, >>>> it was decided that it will overcomplicate it. >>>> It was discussed when new offload API is introduced. >>>> Logging is helpful sometimes, but it is not a panacea. >>> >>> I remember it has been discussed in the context of CRC, because some >>> HW were not >>> able to disable CRC STRIP, it has been implemented as I suggested for >>> this case, >>> when application requests to disable CRC STRIP offload, the PMD print a >>> log >>> saying it can't be disabled and keep the internal state as offload >>> enabled. >>> >>> Other than this I think all offloads announced as supported by PMD can >>> be >>> enabled and disabled. >>> >>>> >>>>>>>>>> If driver see benefits from disabling the offload (e.g. avoid >>> delivery >>>>>>>>>> of RSS hash from NIC to host), it can do it after the patchset. >>>>>>>>> Yes but I think this patchset shouldn't ignore that disabling the >>>>>>> feature is not >>>>>>>>> implemented yet. If those PMDs that has been updated to >>> report >>>>>>> the HASH >>>>>>>>> capability has RSS enabled by default, I suggest adding a check >>> for >>>>>>> this offload >>>>>>>>> in PMD, >>>>>>>>> if it is requested to disable (which means not requested for >>> enable), >>>>>>> print a >>>>>>>>> log saying disabling HASH is not supported and set this flag in >>> the >>>>>>> offload >>>>>>>>> configuration to say PMD is configured to calculate the HASH. >>>>>>>>> Later PMD maintainers may prefer to replace that error log with >>>>>>> actual disable code. >>>>>>>> It is possible to do. Of course, it is better to provide real offload >>>>>>>> values on get, but >>>>>>>> eth_conf is const in rte_eth_dev_configure(), so, we can't >>> change it >>>>>>> and >>>>>>>> it is good. >>>>>>>> So, the only way is rte_eth_rx_queue_info_get(). >>>>>>>> I guess there is a lot of space for the same improvement for >>> other Rx >>>>>>>> offloads >>>>>>>> in various PMDs. >>>>>>> >>>>>>> We don't need the update 'eth_conf' parameter of the >>>>>>> 'rte_eth_dev_configure()', >>>>>>> that is what user requested, but config stored in 'dev->data- >>>> dev_conf' >>>>>>> which >>>>>>> can be updated. >>>> >>>> What for? As I understand data->dev_conf should not be used >>> outside >>>> librte_ethdev and drivers. Basically it means that all patched drivers >>>> should be updated to log information message if the offload is not >>>> requested, but will be enabled anyway and updated >>>> data->dev_conf.rxmode.offloads. Please, confirm. >>> >>> To keep PMD internal state correct. >>> >>>> >>>>>>>> Also I worry that it could be not that trivial to do in all effected >>> PMDs. >>>>>>> >>>>>>> Yes it can be some work, and if this patchset doesn't do it, who >>> will do >>>>>>> the work? >>>> >>>> Pavan, will you do or should I care about it if confirmed? >> >> I will send a v13. >> >>>> >>>>>>>>>>> Specific to `DEV_RX_OFFLOAD_RSS_HASH`, we have already >>>>>>> some RSS config >>>>>>>>>>> structures and it is part of the 'rte_eth_dev_configure()' API, >>>>>>> won't it create >>>>>>>>>>> multiple way to do same thing? >>>>>>>>>> No, a new offload is responsible for RSS hash delivery from >>> NIC to >>>>>>> host >>>>>>>>>> and fill in in mbuf returned to application on Rx. >>>>>>>>> What you have described is already happening without the >>> new >>>>>>> offload flag and >>>>>>>>> this is my concern that we are duplicating it. >>>>>>>>> >>>>>>>>> >>>>>>>>> There is a 'struct rte_eth_rxmode' (under 'struct rte_eth_conf') >>>>>>>>> which has 'enum rte_eth_rx_mq_mode mq_mode;' >>>>>>>>> >>>>>>>>> If "mq_mode == ETH_MQ_RX_NONE" hash calculation is >>> disabled, >>>>>>> and >>>>>>>>> 'mbuf::hash::rss' is not updated. >>>>>>>> No-no. It binds RSS distribution and hash delivery. What the new >>>>>>>> offload allows to achieve: I want Rx to spread traffic over many >>> Rx >>>>>>>> queues, but I don't need RSS hash. >>>>>>> >>>>>>> I see, so RSS configuration will stay same, but driver needs to take >>> care >>>>>>> the >>>>>>> new flags to decide to update or not the mbuf::rss::hash field. >>>>>>> >>>>>>> I don't know if disabling RSS but calculating hash is supported, if >>> not >>>>>>> supported that case also should be checked by driver. >>>> >>>> Yes, it is interesting question what should happen if >>> ETH_MQ_RX_NONE with >>>> DEV_RX_OFFLOAD_RSS_HASH. It sounds like rte_ethdev should >>> check >>>> dev_conf.rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG is set when >>>> DEV_RX_OFFLOAD_RSS_HASH is requested. Otherwise the request is >>> invalid. >>> >>> +1 >>> >>>> May be it could be relaxed in the future, but not now. >>>> >>>>>>>>> (Thanks Bruce to helping finding it out) >>>>>>>>> >>>>>>>>> >>>>>>>>>>> And for the `ol_flags:PKT_RX_RSS` flag, it was already used >>> to >>>>>>> mark that >>>>>>>>>>> 'mbuf::hash::rss' is valid, right? Is there anything new related >>> that >>>>>>> in the set? >>>>>>>>>> As I understand you mean, ol_flags::PKT_RX_RSS_HASH. >>>>>>>>>> Yes, the new offload allows say if application needs it or now. >>>>>>>>>> Basically it decouples RSS distribution and hash delivery. >>>>>>>>> Setting 'ol_flags::PKT_RX_RSS_HASH' and 'mbuf::hash::rss' >>> already >>>>>>> there and not >>>>>>>>> changing. I just want to clarify since this is not clear in the >>> commit log. >>>>>>>>> >>>>>>>>> Only addition is to add a new flag to control PMD to >>> enable/disable >>>>>>> hash >>>>>>>>> calculation (which PMDs ignore in the patch ???) >>>>>>>> It is not calculation, but delivery of the value from HW to >>> applications. >>>>>>> >>>>>>> OK >>>>>>> >>>>>>>> Yes, commit log may/should be improved.> >>>>>>>>>>> Specific to the `DEV_RX_OFFLOAD_FLOW_MARK` and >>>>>>> `RTE_FLOW_ACTION_FLAG`, they are >>>>>>>>>>> rte_flow actions, application can verify and later request >>> these >>>>>>> actions via >>>>>>>>>>> rte_flow APIs. Why we are adding an additional >>> RX_OFFLOAD flag >>>>>>> for them? >>>>>>>>>> The reason is basically the same as above. HW needs to know >>> in >>>>>>> advance, >>>>>>>>>> if application is going to use flow marks and configure Rx >>> queue to >>>>>>> enable >>>>>>>>>> the information delivery. >>>>>>>>> What you described is done via 'rte_flow_create()' API, >>> application >>>>>>> will request >>>>>>>>> those actions via API and Rx queue will be configured >>> accordingly, >>>>>>> this is more >>>>>>>>> dynamic approach. Why application need to set this additional >>>>>>> configuration flag? >>>>>>>> More dynamic approach is definitely better, but it is not always >>>>>>> possible. >>>>>>>> Some PMDs can't even change MTU dynamically or MTU >>> changing >>>>>>> requires >>>>>>>> restart which is hardly really a dynamic change. Of course, it is >>>>>>>> unlikely that >>>>>>>> MTU is changed when traffic is running etc, but still possible. >>>>>>>> The information about necessity to support flow marks delivery >>> may >>>>>>>> be required on Rx queue setup and cannot be changed >>> dynamically >>>>>>> when >>>>>>>> Rx queue is running and application would like to add flow rule >>> with >>>>>>> mark >>>>>>>> action. >>>>>>> It doesn't need to be changed dynamically, application can call >>>>>>> 'rte_flow_validate()' and learn if it can set this action or not. >>> Perhaps I >>>>>>> am >>>>>>> missing something, when it is required to have this as >>> configuration >>>>>>> option? >>>>>>> >>>>>>>>> And as above the new RX offload flags ignored by PMDs, hard >>> to >>>>>>> understand what >>>>>>>>> is the intention here. >>>>>>>>> >>>>>>>>> >>>>>>>>> Above usage of flags feels like the intention is adding some >>> capability >>>>>>>>> information for the PMDs more that adding new offload >>>>>>> configuration. >>>>>>>>> If so this is bigger/older problem, and instead of abusing the >>> offload >>>>>>> flags we >>>>>>>>> can think of an API to present device capabilities, and move >>>>>>> features.ini >>>>>>>>> content to the API in long term. >>>>>>>> What I really like with these new offload flags for Rx hash and >>> flow >>>>>>> mark is >>>>>>>> that it makes features which provide information in mbuf on Rx >>>>>>> consistent: >>>>>>>> - want timestamp? => DEV_RX_OFFLOAD_TIMESTAMP >>>>>>>> - want Rx checksum flags => DEV_RX_OFFLOAD_CHECKSUM >>>>>>>> - want to strip VLAN? => DEV_RX_OFFLOAD_VLAN_STRIP >>>>>>>> - want RSS hash? => DEV_RX_OFFLOAD_RSS_HASH >>>>>>>> - want flow mark support? => >>> DEV_RX_OFFLOAD_FLOW_MARK >>>>>>>> >>>>>>>> Also it perfectly fits dynamic mbuf fields and allows to make RSS >>> hash >>>>>>>> and flow mark fields dynamic with the new offloads as controls >>>>>>> Agree RSS_HASH fits well, my main concern with the patchset is >>> driver >>>>>>> implementations are missing and just ignored. >>>>>>> >>>>>> Ignoring driver implementation is intentional as it involves adding a >>> branch >>>>>> in Rx fastpath function for all drivers and might have -ve effects on >>> performance. >>>>> Yes it may affect performance. Also it may be too much driver >>> specific >>>>> implementation. >>>>> >>>>> That is why I suggest, following: >>>>> For the drivers that claim this capability, >>>>> - For the case driver updates the mbuf::rss:hash >>>>> Check if this offload requested or not, if not print an error and set >>> internal >>>>> config as this offload enabled >>>> >>>> I would not say it is an error. At maximum it is warning, but I would >>> use >>>> just info log level. I think there is nothing critical there. >>> >>> Any log is OK, only info level my be disabled by default by many drivers, >>> to be >>> sure this is printed, warning can be better. >>> >>>> >>>>> - For the case driver not updates the mbuf::rss:hash >>>>> Check if this offload requested or not, if requested print an error >>> and set >>>>> internal config as this offload disabled >>>> >>>> If so, the offload is not advertised and generic checks in rte_ethdev >>>> catch it and return error. >>> >>> You are right, for this case it shouldn't be advertised. >>> >>> >>> Just to double check the understanding of this discussion, is following >>> summary >>> correct? >>> >>> A) When RSS enabled >>> 1) if RSS_HASH set, put hash value to "mbuf::rss:hash" and set >>> PKT_RX_RSS_HASH >>> 2) if RSS_HASH unset, don't updated >>> "mbuf::rss:hash"/PKT_RX_RSS_HASH >>> >>> B) When RSS disabled >>> 3) if RSS_HASH set, >>> i) if HW supports HASH calculation offload independently, enable it >>> and put hash value to "mbuf::rss:hash" and set PKT_RX_RSS_HASH >>> ii) if HW doesn't support it return error >>> 4) if RSS_HASH unset, don't updated >>> "mbuf::rss:hash"/PKT_RX_RSS_HASH >>> >>> >>> This may be good for (3) to enable HASH calculation offload >>> independent from >>> RSS. Not sure if this is supported by HW or this is the motivation of the >>> patch. >>> >>> But for (1), since the hash value is already part of descriptor, this will >>> bring >>> an additional check, as Pavan mentioned, which is not good. >>> >>> For (3) is there any intention to implicitly enable RSS, this was my >>> understanding and concern to have two different methods to enable >>> RSS, >>> I guess my understanding was wrong but I want to confirm this. >>> >>> Currently RSS hash delivery is implied when RSS is enabled. >>> >> >> Correct me if I'm wrong but current agreement is to print a log message >> when application >> tries to disable RSS and it's not supported by underlying PMD. > > Yes. > >> So all the PMD which we aren’t currently modifying need to be have a log >> message. > > No.
If you mean all PMD that is announcing the capability, yes, as you said. I thought you are referring to other PMDs too. > If a PMD not announced this capability, it doesn't need to check this flag. > From > the PMDs that announce this capability, the ones that doesn't support > disabling > the offload should log this and update the PMD config to represent the actual > status. > >> Also, it applies to FLOW_FLAG and FLOW_MARK. > > I am not really sure about them, I think we are duplicating the rte_flow API > and > we shouldn't have them as Rx offload, I would like to hear more comment about > this. > >> >> If we are at an agreement I will send v13. >> >> Thanks, >> Pavan. >> >>>> >>>>> Later PMD maintainers may prefer to replace those errors with >>> actual >>>>> implementation if they want. >>>> >>>> [snip] >>>> >> >
