From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 581A9A0524; Tue, 20 Apr 2021 03:06:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 32D4541448; Tue, 20 Apr 2021 03:06:07 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 4160640685 for ; Tue, 20 Apr 2021 03:06:05 +0200 (CEST) IronPort-SDR: Mq2dX6T76ErGo5LV485wdx88IiQdG59ORAZcPiwWscDqSlJLGgWRAiZJE/rqgDtj5Jj5HjnpS5 cte4H14GGe5w== X-IronPort-AV: E=McAfee;i="6200,9189,9959"; a="256734204" X-IronPort-AV: E=Sophos;i="5.82,235,1613462400"; d="scan'208";a="256734204" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Apr 2021 18:06:02 -0700 IronPort-SDR: jy3wMtNIbDo4P+EbVn6rwtIwVYIkjMhR7hnT+LZNryOYHYwTlBsTiTr7JUendOda98l82ZAxxI eo+cCPWhaPSg== X-IronPort-AV: E=Sophos;i="5.82,235,1613462400"; d="scan'208";a="452328703" Received: from agrady-mobl.ger.corp.intel.com (HELO [10.213.224.96]) ([10.213.224.96]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Apr 2021 18:05:59 -0700 To: Thomas Monjalon , Andrew Rybchenko Cc: Ori Kam , "dev@dpdk.org" , "pbhagavatula@marvell.com" , "jerinj@marvell.com" , John McNamara , Marko Kovacevic , Adrien Mazarguil , "david.marchand@redhat.com" , "ktraynor@redhat.com" , Olivier Matz , Raslan Darawsheh , Qi Zhang References: <20191025152142.12887-1-pbhagavatula@marvell.com> <20e51141-591e-0620-8ec6-7059e588009c@intel.com> <3773300.PCdar4H6sl@thomas> From: Ferruh Yigit X-User: ferruhy Message-ID: <835cce38-3a83-41b9-d619-3d349d0164eb@intel.com> Date: Tue, 20 Apr 2021 02:05:55 +0100 MIME-Version: 1.0 In-Reply-To: <3773300.PCdar4H6sl@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add flow action type update as an offload X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 2/17/2021 2:10 PM, Thomas Monjalon wrote: > 17/02/2021 14:45, Ferruh Yigit: >> On 7/3/2020 3:34 PM, Ferruh Yigit wrote: >>> On 11/19/2019 11:09 AM, Thomas Monjalon wrote: >>>> 19/11/2019 11:59, Andrew Rybchenko: >>>>> On 11/19/19 12:50 PM, Thomas Monjalon wrote: >>>>>> 19/11/2019 10:24, Andrew Rybchenko: >>>>>>> On 11/8/19 4:30 PM, Thomas Monjalon wrote: >>>>>>>> 08/11/2019 14:27, Andrew Rybchenko: >>>>>>>>> On 11/8/19 4:17 PM, Thomas Monjalon wrote: >>>>>>>>>> 08/11/2019 13:00, Andrew Rybchenko: >>>>>>>>>>> On 11/8/19 2:03 PM, Thomas Monjalon wrote: >>>>>>>>>>>> 08/11/2019 11:42, Andrew Rybchenko: >>>>>>>>>>>>> On 11/8/19 1:28 PM, Thomas Monjalon wrote: >>>>>>>>>>>>>> 08/11/2019 09:35, Andrew Rybchenko: >>>>>>>>>>>>>>> The problem: >>>>>>>>>>>>>>> ~~~~~~~~~~~~ >>>>>>>>>>>>>>> PMD wants to know before port start if application wants to >>>>>>>>>>>>>>> to use flow MARK/FLAG in the future. It is required because: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 1. HW may be configured in a different way to reserve resources >>>>>>>>>>>>>>> for MARK/FLAG delivery >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2. Datapath implementation choice may depend on it (e.g. vPMD >>>>>>>>>>>>>>> is faster, but does not support MARK) >>>>>>>>>>>>>> Thank you for the clear problem statement. >>>>>>>>>>>>>> I agree with it. This is a real design issue. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Discussed solutions: >>>>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~~~ >>>>>>>>>>>>> May be it is not 100% clear since below are alternatives. >>>>>>>>>>>>> >>>>>>>>>>>>>>> A. Explicit Rx offload suggested by the patch. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> B. Implicit by validation of a flow rule with MARK/FLAG actions used. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> C. Use dynamic field/flag (i.e. application registers dynamic field >>>>>>>>>>>>>>> and/or flag and PMD uses lookup to solve the problem) plus part >>>>>>>>>>>>>>> of (B) to discover if a feature is supported. >>>>>>>>>>>>>> The dynamic field should be registered via a new API function >>>>>>>>>>>>>> named '_init'. >>>>>>>>>>>>>> It means the application must explicit request the feature. >>>>>>>>>>>>>> I agree this is the way to go. >>>>>>>>>>>>> If I understand your statement correctly, but (C) is not ideal since it >>>>>>>>>>>>> looks global. If registered dynamic field of mbuf and is flag that >>>>>>>>>>>>> the feature should be enabled, it is a flag to all ports/devices. >>>>>>>>>>>>> >>>>>>>>>>>>>>> All solutions require changes in applications which use these >>>>>>>>>>>>>>> features. There is a deprecation notice in place which advertises >>>>>>>>>>>>>>> DEV_RX_OFFLOAD_FLOW_MARK addition, but may be it is OK to substitute >>>>>>>>>>>>>>> it with solution (B) or (C). Solution (C) requires changes since >>>>>>>>>>>>>>> it should be combined with (B) in order to understand if >>>>>>>>>>>>>>> the feature is supported. >>>>>>>>>>>>>> I don't understand. >>>>>>>>>>>>>> Application request and PMD support are two different things. >>>>>>>>>>>>>> PMD support must be via rte_flow validation on a case by case anyway. >>>>>>>>>>>>> I mean that application wants to understand if the feature is >>>>>>>>>>>>> supported. Then, it wants to enable it. In the case of (B), >>>>>>>>>>>>> if I understand the solution correctly, there is no explicit >>>>>>>>>>>>> way to enable, PMD just detects it because of discovery is done >>>>>>>>>>>>> (that's what I mean by "implicit" and it is a drawback from my >>>>>>>>>>>>> point of view, but still could be considered). (C) solves the >>>>>>>>>>>>> problem of (B). >>>>>>>>>>>>> >>>>>>>>>>>>>>> Advantages and drawbacks of solutions: >>>>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>>>>>>>>>> 1. The main drawback of (A) is a "duplication" since we already >>>>>>>>>>>>>>> have a way to request flow MARK using rte_flow API. >>>>>>>>>>>>>>> I don't fully agree that it is a duplication, but I agree >>>>>>>>>>>>>>> that it sounds like duplication and complicates a bit flow >>>>>>>>>>>>>>> MARK usage by applications. (B) complicates it as well. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2. One more drawback of the solution (A) is the necessity of >>>>>>>>>>>>>>> similar solution for META and it eats one more offload bit. >>>>>>>>>>>>>>> Yes, that's true and I think it is not a problem. >>>>>>>>>>>>>>> It would make it easier for applications to find out if >>>>>>>>>>>>>>> either MARK or META is supported. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 3. The main advantage of the solution (A) is simplicity. >>>>>>>>>>>>>>> It is simple for application to understand if it supported. >>>>>>>>>>>>>>> It is simple in PMD to understand that it is required. >>>>>>>>>>>>>>> It is simple to disable it - just reconfigure. >>>>>>>>>>>>>>> Also it is easier to document it - just mention that >>>>>>>>>>>>>>> the offload should be supported and enabled. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 4. The main advantage of the solution (B) is no "duplication". >>>>>>>>>>>>>>> I agree that it is valid argument. Solving the problem >>>>>>>>>>>>>>> without extra entities is always nice, but unfortunately >>>>>>>>>>>>>>> it is too complex in this case. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 5. The main drawback of the solution (B) is the complexity. >>>>>>>>>>>>>>> It is necessary to choose a flow rule which should be used >>>>>>>>>>>>>>> as a criteria. It could be hardware dependent. >>>>>>>>>>>>>>> Complex logic is require in PMD if it wants to address the >>>>>>>>>>>>>>> problem and control MARK delivery based on validated flow >>>>>>>>>>>>>>> rules. It adds dependency between start/stop processing and >>>>>>>>>>>>>>> flow rules validation code. >>>>>>>>>>>>>>> It is pretty complicated to document it. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 6. Useless enabling of the offload in the case of solution (A) >>>>>>>>>>>>>>> if really used flow rules do not support MARK looks like >>>>>>>>>>>>>>> drawback as well, but easily mitigated by a combination >>>>>>>>>>>>>>> with solution (B) and only required if the application wants >>>>>>>>>>>>>>> to dive in the level of optimization and complexity and >>>>>>>>>>>>>>> makes sense if application knows required flow rules in >>>>>>>>>>>>>>> advance. So, it is not a problem in this case. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 7. Solution (C) has drawbacks of the solution (B) for >>>>>>>>>>>>>>> applications to understand if these features are supported, >>>>>>>>>>>>>>> but no drawbacks in PMD, since explicit criteria is used to >>>>>>>>>>>>>>> enable/disable (dynamic field/flag lookup). >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 8. Solution (C) is nice since it avoids "duplication". >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 9. The main drawback of the solution (C) is asymmetry. >>>>>>>>>>>>>>> As it was discussed in the case of RX_TIMESTAMP >>>>>>>>>>>>>>> (if I remember it correctly): >>>>>>>>>>>>>>> - PMD advertises RX_TIMESTAMP offload capability >>>>>>>>>>>>>>> - application enables the offload >>>>>>>>>>>>>>> - PMD registers dynamic field for timestamp >>>>>>>>>>>>>>> Solution (C): >>>>>>>>>>>>>>> - PMD advertises nothing >>>>>>>>>>>>>>> - application uses solution (B) to understand if >>>>>>>>>>>>>>> these features are supported >>>>>>>>>>>>>>> - application registers dynamic field/flag >>>>>>>>>>>>>>> - PMD does lookup and solve the problem >>>>>>>>>>>>>>> The asymmetry could be partially mitigated if RX_TIMESTAMP >>>>>>>>>>>>>>> solution is changed to require an application to register >>>>>>>>>>>>>>> dynamic fields and PMD to do lookup if the offload is >>>>>>>>>>>>>>> enabled. So, the only difference will be in no offload >>>>>>>>>>>>>>> in the case of flow MARK/FLAG and usage of complex logic >>>>>>>>>>>>>>> to understand if it is supported or no. >>>>>>>>>>>>>>> May be it would be really good since it will allow to >>>>>>>>>>>>>>> have dynamic fields registered before mempool population. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 10. Common drawback of solutions (B) and (C) is no granularity. >>>>>>>>>>>>>>> Solution (A) may be per queue while (B) and (C) cannot be >>>>>>>>>>>>>>> per queue. Moreover (C) looks global - for all devices. >>>>>>>>>>>>>>> It could be really painful. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> (C) is nice, but I still vote for simplicity and >>>>>>>>>>>>>>> granularity of (A). >>>>>>>>>>>>>> I vote for clear separation of application needs and PMD support, >>>>>>>>>>>>>> by using the method C (dynamic fields). >>>>>>>>>>>>>> I agree timestamp must use the same path. >>>>>>>>>>>>>> I agree it's complicate because we don't know in advance whether >>>>>>>>>>>>>> a flow rule will be accepted, but that's the reality, config is complex. >>>>>>>>>>>>> Do you think that global nature of the (C) is acceptable? >>>>>>>>>>>> That's a good question. >>>>>>>>>>>> Maybe the feature request should be per port. >>>>>>>>>>>> In this case, we are back to solution A with a flag per port? >>>>>>>>>>> Offloads are natively per-queue as well, so (A) keeps the choice >>>>>>>>>>> between per-port vs per-queue to PMDs as usual. >>>>>>>>>>> >>>>>>>>>>>> Note that A and C will not guarantee that the offload will be possible. >>>>>>>>>>> Yes, definitely. >>>>>>>>>>> >>>>>>>>>>>> We need B (flow rule validation) anyway. >>>>>>>>>>> Strictly speaking (B) (checking flow rules before device >>>>>>>>>>> startup) is required if an application can predict flow >>>>>>>>>>> rules and wants to ensure that MARK offload will be usable. >>>>>>>>>>> Otherwise, it may be skipped. >>>>>>>>>> No no, I mean flow rule validation MUST be used anyway >>>>>>>>>> during the runtime before applying a rule. >>>>>>>>>> I agree it is hard to predict. I speak only about real rules. >>>>>>>>> OK, I see. Of course, flow rule validation is required at runtime. >>>>>>>>> I was rather concentrated on the stated problem solutions. >>>>>>>>> >>>>>>>>>>>> It seems A, B, C are not alternatives but all required >>>>>>>>>>>> as pieces of a puzzle... >>>>>>>>>>> Unfortunately true in the most complex case. >>>>>>>>>>> Right now it will be A with B if required as explained above. >>>>>>>>>>> C will come a bit later when the field migrates to dynamic. >>>>>>>>>>> >>>>>>>>>>> May be it is even better if application registers dynamic >>>>>>>>>>> fields before an attempt to enable offload to be sure that >>>>>>>>>>> it will not fail because of impossibility to register >>>>>>>>>>> dynamic field (lack of space). I'm not sure, but it is not >>>>>>>>>>> not that important. >>>>>>>>>> Yes of course, lack of mbuf space is another reason for >>>>>>>>>> disabling the feature. >>>>>>>>>> >>>>>>>>>>> If we finally go way A, should we add offloads for META back? >>>>>>>>>>> I guess separate Rx and Tx are required. >>>>>>>>>> I would prefer to add it as dynamic flags. >>>>>>>>>> Why rushing on a very temporary solution while it is not a new issue? >>>>>>>>> Basically it means that we go just (B)+(C) in the case of META. >>>>>>>>> I have no strong opinion but thought that it could be better to >>>>>>>>> align the solution. Of course, we can wait with it. As I understand >>>>>>>>> META is an experimental feature. >>>>>>>> Yes it is experimental and I think it is too late to align now. >>>>>>>> >>>>>>>> Anyway, we will probably to discuss again these offloads TAG/MARK/META, >>>>>>>> as requested by several people. >>>>>>>> >>>>>>> The series implements (A) to help to solve the problem described above. >>>>>>> What is the fate of the series in v19.11 in accordance with the >>>>>>> discussion? >>>>>> I am against adding anything related to a feature union'ed in mbuf. >>>>>> The feature must move to dynamic field first. >>>>>> >>>>>> In addition, such capability is very weak. >>>>>> I am not sure it is a good idea to have some weak capabilities, >>>>>> meaning a feature could be available but not in all cases. >>>>>> I think we should discuss more generally how we want to handle >>>>>> the rte_flow capabilities conveniently and reliably. >>>>> >>>>> It is really unexpected outcome from the above discussion. >>>> >>>> I'm sorry, I thought I was clear in my request to switch to dynamic first. >>>> >>>> >>>>> It is just possibility to deliver and handle marks on datapath and >>>>> request to have it. It says almost nothing about rte_flow rules >>>>> supported etc. I'll be happy to take part in the discussion. >>>>> >>>>>> So regarding 19.11, as this feature is not new, it can wait 20.02. >>>>> >>>>> OK, it is not critical for me, so I don't mind, however, I've seen >>>>> patches which try to use it [1] except net/octeontx2 in the second >>>>> patch of the series. >>>>> >>>>> [1] https://patches.dpdk.org/patch/62415/ >>>> >>> >>> Sorry, I have to resurrect this old (long) discussion because the patches are >>> still active in the patchwork [1] and the deprecation notice is still there [2]. >>> >>> Andrew has a good summary in the thread [3], after a year nothing seems changed. >>> >>> >>> Pavan, Thomas, Andrew, Ori, >>> >>> What is our plan with this series, lets try to have a conclusion. >>> >>> >>> >>> >>> [1] >>> https://patches.dpdk.org/user/todo/dpdk/?series=7076 >>> >>> [2] >>> http://lxr.dpdk.org/dpdk/v20.05/source/doc/guides/rel_notes/deprecation.rst#L88 >>> >>> [3] >>> http://inbox.dpdk.org/dev/f170105b-9c60-1b04-cb18-52e0951ddcdb@solarflare.com/ >>> >>> >> >> I re-read the thread, will try to have a little movement while we are in the new >> release cycle, if there is no update I am planning to reject the patches. >> >> There seems two problems: >> >> P1) Application will keep trying to program NIC for MARK action for each flow, >> since application doesn't know if next one will succeed or not. >> If only there would be a way to find out that NIC/PMD doesn't support the MARK >> action at all, this could save application to keep trying. >> >> P2) PMD can make better internal choices if it gets more hint from application >> about MARK action may be used or not. >> Application at least may say it won't use the MARK flow action at all. >> >> >> This patch uses offload flags infrastructure to solve above two problems, >> solution (A) in Andrew's summary. >> >> Although it may solve the issues, there are questions/concerns around using this >> additional flag to control flow API, I also agree it may be confusing in the >> design level although practically using flags can be simple. >> And this is not generic solution, what happen with META action question is >> already hanging on in the thread, more flags? How many more can we add? >> >> And also there is option an to use dynamic mbuf flags to detect the capability, >> solution (C) in Andrew's summary, again it may solve the problem but it looks >> again a workaround to solve same flow API design restriction, and this one is >> not as simple as (A). >> >> Overall the discussion seems going on circles without an agreed on decision. >> >> >> >> What about trying to solve this with flow API return values, >> >> If a flow rule is not supported at all by the NIC/PMD, it may return >> '-ENO_WAY_JOSE', and application knows it can't be used at all, this may solve >> the (P1) above. > > I like it, but who is Jose? > We can also have a function to test if an action is supported or not at all. > >> And if a flow rule can be supported for the given pattern, but it is not >> supported right now because current configuration or resourcing restrictions >> doesn't allow creating rule, a special error type can be returned with a >> descriptive error log for application to response: >> -ECONFLICT, "Can't enable rule A when rule B is enabled" >> -EDATAPATH, "Can't enable this rule when vector datapath is used" >> -ERESOURCE, "Can't enable more than 3 rules" >> This may solve the (P2) partially. >> >> I am not sure about second part, but at least first part shouldn't be too hard >> to implement, and it is a generic solution, what do you think? > > +1 > This is an old patch, and there was no update/response during the release time-frame. I am rejecting in patchwork, please send a new version if there is still demand for it.