From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5E65CA31F3 for ; Fri, 18 Oct 2019 12:31:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9C0211D158; Fri, 18 Oct 2019 12:31:39 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 242051D151 for ; Fri, 18 Oct 2019 12:31:38 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us5.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 4DCDF4C0066; Fri, 18 Oct 2019 10:31:36 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 18 Oct 2019 03:31:31 -0700 To: Ferruh Yigit , , CC: , Adrien Mazarguil , "Thomas Monjalon" , Xiaolong Ye , "Bruce Richardson" References: <20191010105130.3382-1-pbhagavatula@marvell.com> <20191017120245.984-1-pbhagavatula@marvell.com> <868c4186-a899-3974-ce1a-9d0d395b2f81@intel.com> <1701432f-d080-8772-3612-3710f0f9eae0@intel.com> From: Andrew Rybchenko Message-ID: <3251fc00-7598-1c4f-fc2a-380065f0a435@solarflare.com> Date: Fri, 18 Oct 2019 13:31:29 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <1701432f-d080-8772-3612-3710f0f9eae0@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ocex03.SolarFlarecom.com (10.20.40.36) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24984.005 X-TM-AS-Result: No-12.680700-4.000000-10 X-TMASE-MatchedRID: yebcs53SkkD4ECMHJTM/ufZvT2zYoYOwC/ExpXrHizxwkdIrVt8X1cG1 8AJ4wbudFhMAXiSJc3Av3RWjRyG6TtF/tAhYTSzX34b00P59Zxm3xwqfnvnKHny/Hx1AgJrrstf L6WfCYzEoFhurGV67Z11A7/0AFfe7GXC6wGf12RWqNnzrkU+2mvsJB65N+sbMSX8n1Gj4wAEBSr A72lIhPcKRiXfaKYmpV5ch7TAAY1TVqiuVd9LMeRlJRfzNw8afGUqOjzOC7Ip7U6ND9bLE/m2rD qsRwTo30P8y4NB7uOvvpVnoxBz4V/LETRWl+wGPzNIobH2DzGFJaD67iKvY01n4z0T8fHooLYIB beTLxavVrOiukcVwyQzQDh2ICYdn34rFJ+CwBsd6UYddkosvaw+jS+LRpl81sS0sZEB7c8baRsc iQavS5olkruB0uXxQ9YGZ92R+qhsu1SW09WHtVo6cpbnLdja9BGvINcfHqhcAjiw/nJICh+CP7Y GHp9vlL0bq/GMG9h4kBeoLMVXrsw/m8ZR5c3818KQMqZXaCzkpWss5kPUFdOJiUqjHevI0akCrX hKzdtkaJ40oN2kAoWUcBLnnArg3iRmbxPnK0cz0VCHd+VQiHplP7dABOvkyKwv5G6fLcA9JcBXv KYkUg32B9Glgn0cbJ7OQgP38hHGescXXGhKTEvVY7U3NX8JgY9FA+DrisUqXPCOB3g3KS/I8kvc lw7r9ipu/VRQCOX/dN8J+VMyke0y6jREa0tRrLyz9QvAyHjoXw5Hb3/XOHcsh83hywc54y5aOmZ vTmVPzJ1FYvQ1M8R3kdXUuScbhU0y1hIXcU3eeAiCmPx4NwFkMvWAuahr8+gD2vYtOFhgqtq5d3 cxkNWCo5XYSv90eaisGP6ikIqPk32P5sazKUSWp5C7h4dY7BRjf7ZkLGqg= X-TM-AS-User-Approved-Sender: No X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--12.680700-4.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24984.005 X-MDID: 1571394697-yVzkUeJJLr2W Subject: Re: [dpdk-dev] [PATCH v12 0/7] ethdev: add new Rx offload flags X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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, pbhagavatula@marvell.com wrote: >>>> From: Pavan Nikhilesh >>>> >>>> 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. > 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. >> 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. Also I worry that it could be not that trivial to do in all effected PMDs. >>> 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. > (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. 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. > 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. >>>> 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 >>> <...>