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. > >> >>>> 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. > >> 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? > >> >>>>> 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. > >> >>> (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. >I am not so sure about FLOW_MARK one, it looks like it is duplicating the >rte_flow API. > >> >>>>>> Add new packet type set function >`rte_eth_dev_set_supported_ptypes`, >>>>>> allows application to inform PMDs about the packet types it is >interested >>>>>> in. Based on ptypes requested by application PMDs can >optimize the Rx path. >>>>> OK to the API, but why "Packet type parsing" feature updated to >say it should >>>>> implement this API? >>>>> Is this API really required to say "Packet type parsing" supported >by PMD? >>>> As I understand it is not strictly required, but related to the feature. >>> I am OK with "related", but it is documented as "implements", so doc >says it is >>> required. >> >> Agreed. >> >>>>>> For example, if a given PMD doesn't support any packet types >that the >>>>>> application is interested in then the application can disable[1] >writes to >>>>>> `mbuf.packet_type` done by the PMD and use a software >ptype parser. >>>>>> [1] rte_eth_dev_set_supported_ptypes(*port_id*, >RTE_PTYPE_UNKNOWN, >>>>>> NULL, 0); >>>>> And for the 7/7 patch, why we are updating all examples, is the >API something do >>>>> we really need to call for any DPDK application? I am for leaving >the default >>>>> behavior unless there is a very specific case for set or disable >packet typing. >>>>> Instead implement a command in testpmd to test this feature. >>>> If an application does not use packet types provided in mbuf, it is >>>> better to inform PMD that the information is not required to allow >PMD >>>> to do optimizations. >>>> >>> I can see disabling packet type detection may increase the >performance but >>> sample applications are to demonstrate a specific feature, adding >these kind of >>> APIs will pollute them. >>> 'skeleton' app that shows the most basic code for forwarding >sample, why it is >>> now having "experimental" 'set_supported_ptypes()' API? Same for >other. As said >>> before I think a testpmd command suits better here. >> >> May be you're right and we should reconsider which applications >> are updated and which are ignored. I guess before the criteria >> was simple: don't use packet type information, say so to take >> benefits from all possible optimizations. >> >>>> Yes, may be it would be better to have it as the >>>> default behaviour, but it would be behaviour change in comparison >>>> to previous DPDK releases and it is better to avoid it. >>> Sorry I missed why not calling this function cause behavior change? I >think it >>> is other way around, no? >> >> Just misunderstanding. What I was trying to say is that it could >> be more logical to have packet type parsing and delivery >> disabled by default (as we have for all other offloads), but >> it would be behaviour change from application point of view. >> That's why it is necessary to disable explicitly. >> >> Thanks, >> Andrew. >> >>>> Thanks, >>>> Andrew. >>>> >>>>>> v12 Changes: >>>>>> ----------- >>>>>> - Rebase onto next-net. >>>>>> >>>>>> v11 Changes: >>>>>> ----------- >>>>>> - Use RTE_DIM to get array size. >>>>>> - Since we are using a list of MASKs to validate ptype_mask >return -EINVAL >>>>>> if any unknown mask is set. >>>>>> - Rebase to TOT. >>>>>> >>>>>> v10 Changes: >>>>>> ----------- >>>>>> - Modify ptype_mask validation in >set_supported_ptypes.(Andrew) >>>>>> >>>>>> v9 Changes: >>>>>> ---------- >>>>>> - Add ptype_mask validation in set_supported_ptypes.(Andrew) >>>>>> - Make description more verbose. >>>>>> >>>>>> v8 Changes: >>>>>> ---------- >>>>>> - Make description more verbose. >>>>>> - Set RTE_PTYPE_UNKNOWN in set_ptypes array when either >get ot set ptypes >>>>>> is not supported by ethernet device. >>>>>> >>>>>> v7 Changes: >>>>>> ---------- >>>>>> - Fix unused variable in net/octeontx2 >>>>>> >>>>>> v6 Changes: >>>>>> ---------- >>>>>> - Add additional checks for set supported ptypes.(Andrew) >>>>>> - Clarify `rte_eth_dev_set_supported_ptypes` documentation. >>>>>> - Remove DEV_RX_OFFLOAD_FLOW_MARK emulation from >net/octeontx2. >>>>>> >>>>>> v5 Changes: >>>>>> ---------- >>>>>> - Fix typos. >>>>>> >>>>>> v4 Changes: >>>>>> ---------- >>>>>> - Set the last element in set_ptype array as >RTE_PTYPE_UNKNOWN to mark the end >>>>>> of array. >>>>>> - Fix invalid set ptype function call in examples. >>>>>> - Remove setting rte_eth_dev_set_supported_ptypes to >UNKNOWN in l3fwd-power. >>>>>> >>>>>> v3 Changes: >>>>>> ---------- >>>>>> - Add missing release notes. (Andrew) >>>>>> - Re-word various descriptions. >>>>>> - Fix ptype set logic. >>>>>> >>>>>> v2 Changes: >>>>>> ---------- >>>>>> - Update release notes. (Andrew) >>>>>> - Redo commit logs. (Andrew) >>>>>> - Disable ptype parsing for unsupported examples. (Jerin) >>>>>> - Disable RSS write only in generic mode eventdev_pipeline. >(Jerin) >>>>>> - Modify set_supported_ptypes function to return successfuly >set mask >>>>>> instead of failure. >>>>>> - Dropped set_supported_ptypes to drivers by handling in library >>>>>> layer, interested PMD can add it in. >>>>>> >>>>>> Pavan Nikhilesh (7): >>>>>> ethdev: add set ptype function >>>>>> ethdev: add mbuf RSS update as an offload >>>>>> ethdev: add flow action type update as an offload >>>>>> drivers/net: update Rx RSS hash offload capabilities >>>>>> drivers/net: update Rx flow flag and mark capabilities >>>>>> examples/eventdev_pipeline: add new Rx RSS hash offload >>>>>> examples: disable Rx packet type parsing >>>>> <...> >>
