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 9F723A046B for ; Sun, 18 Aug 2019 09:09:04 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 793F91B96B; Sun, 18 Aug 2019 09:09:03 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 7A1A737B7 for ; Sun, 18 Aug 2019 09:09:01 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id ACE69800059; Sun, 18 Aug 2019 07:08:59 +0000 (UTC) Received: from [192.168.1.11] (85.187.13.152) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Sun, 18 Aug 2019 08:08:51 +0100 To: Shahaf Shuler , "pbhagavatula@marvell.com" , "jerinj@marvell.com" , "ferruh.yigit@intel.com" , John McNamara , Marko Kovacevic , Thomas Monjalon CC: "dev@dpdk.org" References: <20190816055511.2322-1-pbhagavatula@marvell.com> <20190816055511.2322-4-pbhagavatula@marvell.com> <4ca361c2-c350-6888-e5c3-5d3454af35ab@solarflare.com> <3e4b3c49-842e-4d99-fdc0-7647a4897296@solarflare.com> From: Andrew Rybchenko Message-ID: Date: Sun, 18 Aug 2019 10:08:47 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Originating-IP: [85.187.13.152] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24854.003 X-TM-AS-Result: No-13.833700-8.000000-10 X-TMASE-MatchedRID: j4nUk6F+aLYeimh1YYHcKPZvT2zYoYOwC/ExpXrHizxG/okVZ6c/dtlM ASzPGs9UeBU1sygo3NXhidLmkO138TZaBU7bpoUbPaBTJyy84wXpVMb1xnESMg/vnJWZIdGAedR sVpejKZ6vEQNFRmBeZI/h+VzXVDnD5neCrx57xxp/OBWacv+iVadlL9piCOvOV8ukjx868O6MFi EZi1VquU/a96gHPa2eQVfk++ALZhc5NXlWWXBFjgCNFKULxGCZyeUl7aCTy8h5r69xGFEnQL2/x r1t+ygm5jr+I6ik278Y+GYXlqwk7V7w4pzklHQxSVHYMTQ1F1pdA4rYaKGKwvyQXCBzKijhyVdG RU34S3zrcZeS7mUy9xoAXSLeXC9wNmnQx++HUgUUmR8f4HPVzqDzzrtsjCZTKwrot6guaXOX0y6 nlwu2zNXGlFGnEwOETP+gXA7v436f2/wegNnQURlJRfzNw8afDxIIGsB6XY3fc2Xd6VJ+yj8nlI qeUd1tdsAYJ19+6cV4A0q9fpAaDbQlCn8RT0gFmOFnGEL0JOP2155bpR+TIBn4299x0aHBJkpEg WlcD4FWkAzeYr3delDuJbxlk5hO1u6BHegn9C9LIfps09VJ28/0TmAtg1KEBCzD0Dc8iUulMx9H LUdHOeWIXxCzEGZiYp4GnDRLRUtTJdKXsqfTQwf11J5hhjjq5GNm5cTRaUcGmHr1eMxt2UAc6Dy oS2rI2lO9wdJIjwWBLdWNzj2myeVbVfdLscbXSvQm6xqmCPIP0R5PvxNzGznZfxjBVQRbwyVH/F DkCpPnzlXMYw4XMAGLeSok4rrZC24oEZ6SpSkgbhiVsIMQK9LdHHLXgng3V14GUzlvpTEQWr/Rc Vo7Xb6dIU0RMaFWrjyCgJBhRJZfCOKFKuVYGg== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--13.833700-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24854.003 X-MDID: 1566112140-RJLOs3llEBCJ Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as an offload 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 8/18/19 9:20 AM, Shahaf Shuler wrote: > Sunday, August 18, 2019 8:57 AM, Andrew Rybchenko: >> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as >> an offload >> >> On 8/18/19 7:59 AM, Shahaf Shuler wrote: >>> Friday, August 16, 2019 11:05 AM, Andrew Rybchenko: >>>> ; Thomas Monjalon >> >>>> Cc: dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type >>>> update as an offload >>>> >>>> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote: >>>>> From: Pavan Nikhilesh >>>>> >>>>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be >>>> used to >>>>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`. >>>> Notes similar to RSS hash. >>>> >>>> It requires better motivation why. It lets Rx queue know that it will >>>> be used as flow action MARK target and the queue should be configured >>>> to deliver the mark from NIC to PMD and processed in the driver. >>> This one is even worse than the RSS (sorry). >>> >>> First - the API breakage exists also here and if we want to include such >> patch we should have proper doc on RN. >> >> Yes, there is a deprecation notice for it in v19.08 and I already mentioned in >> review notes for the patch [1/7] that release notes are required. >> >>> Second - the user explicitly inserted a rte_flow rule w/ action mark. Its >> expectation is to receive his mark on the mbuf (otherwise why would it set >> this action?). it is not expected from the user to set another offload flag just >> to enable the mark set on the mbuf. It makes the user experience very >> convoluted. >>> Third - so far we never reported rte_flow capabilities, and there is a good >> reason for it - the cap matrix is too big. For rte_flow the chosen approach was >> trail and error. If we start w/ the flow mark, the amount of cap bits the >> application will need to monitor will be huge. >> >> The feature differs a lot from other rte_flow features since it adds Rx meta >> information. >> And yes, rte_flow rule to set mark should fail with appropriate diagnostics if >> the offload is supported by not enabled or not supported at all. So, >> application will be informed and I think it is less worse than RSS hash. >> >>> My suggestion here, for PMD that wants to optimize their datapath is to >> check if flow w/ mark action was inserted on the queue. So long there is no >> such flow they can disable the set of the mark. >> >> Unfortunately the information is required on Rx queue setup stage and >> rte_rule insertion is too late. > See my comments on the RSS patch. Same point of discussion. > > The main question we need to answer - > User set flow mark action by rte_flow (that was accepted). Why does it need to set more flag? I think all arguments are on the table. Mine:  - possibility to squeeze more performance from HW and SW  - consistency (at least as I see it) in filling in of the information in mbuf    (direct using corresponding offloads) Your:  - more complexity in control (which is less painful in the case of flow mark,    since it should be an error on flow rule setup, and a bit more painful in    in the case of RSS hash since there is simply no RSS hash if not enabled)  - API breakage (but there is deprecation notice in v19.08) Many thanks for the discussion. >> Anyway, the important point here is all Rx offloads consistency: >> want something - enable it. The only remaining exception is packet type >> which default behaviour is preserved since really flexible solution suggested >> by Konstantin. >> >>>> Also I think that flow API action MARK documentation should be >>>> updated to mentioned the offload. >>>> >>>>> Signed-off-by: Pavan Nikhilesh >>>>> --- >>>>> doc/guides/nics/features.rst | 12 ++++++++++++ >>>>> lib/librte_ethdev/rte_ethdev.h | 1 + >>>>> 2 files changed, 13 insertions(+) >>>>> >>>>> diff --git a/doc/guides/nics/features.rst >>>>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644 >>>>> --- a/doc/guides/nics/features.rst >>>>> +++ b/doc/guides/nics/features.rst >>>>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in. >>>>> * **[provides] mbuf**: ``mbuf.packet_type``. >>>>> >>>>> >>>>> +.. _nic_features_flow_action_type_update: >>>> May be  _nic_features_flow_mark ? >>>> >>>>> + >>>>> +Flow type update >>>> May be "Flow mark delivery" ? >>>> >>>>> +---------------- >>>>> + >>>>> +Supports flow action type update to ``mbuf.ol_flags`` and >>>> ``mbuf.hash.fdir.hi``. >>>>> + >>>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: >>>> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``. >>>> >>>> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE. >>>> >>>> >>>> >>>>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``, >>>>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``, >>>>> + ``mbuf.hash.fdir.hi`` >>>>> + >>>>> + >>>>> .. _nic_features_timesync: >>>>> >>>>> Timesync >>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h >>>>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644 >>>>> --- a/lib/librte_ethdev/rte_ethdev.h >>>>> +++ b/lib/librte_ethdev/rte_ethdev.h >>>>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf { >>>>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000 >>>>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000 >>>>> #define DEV_RX_OFFLOAD_RSS_HASH 0x00080000 >>>>> +#define DEV_RX_OFFLOAD_FLOW_MARK 0x00100000 >>>>> >>>>> #define DEV_RX_OFFLOAD_CHECKSUM >>>> (DEV_RX_OFFLOAD_IPV4_CKSUM | \ >>>>> DEV_RX_OFFLOAD_UDP_CKSUM | \ >>>> Add to rte_rx_offload_names