>-----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. So all the PMD which we aren’t currently modifying need to be have a log message. Also, it applies to FLOW_FLAG and FLOW_MARK. 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] >>
