From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 0B7BA1B4C0 for ; Fri, 5 Apr 2019 13:55: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 (Proofpoint Essentials ESMTP Server) with ESMTPS id F3F3B280053; Fri, 5 Apr 2019 11:54:59 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 5 Apr 2019 12:54:53 +0100 To: Adrien Mazarguil , Ori Kam CC: Dekel Peled , "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "dev@dpdk.org" References: <1553177917-43297-1-git-send-email-dekelp@mellanox.com> <1554218001-62012-2-git-send-email-dekelp@mellanox.com> <20190403091432.GP4889@6wind.com> <20190403124921.GR4889@6wind.com> <20190404132556.GS4889@6wind.com> From: Andrew Rybchenko Message-ID: <55805412-3145-4542-9376-67d02a62fa8b@solarflare.com> Date: Fri, 5 Apr 2019 14:54:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <20190404132556.GS4889@6wind.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] 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-24532.003 X-TM-AS-Result: No-30.718300-8.000000-10 X-TMASE-MatchedRID: B0+RG8xpjtQOwH4pD14DsPHkpkyUphL9gHzgwy8qV5p77G36GYsVE/vt 4ENurW24d3NPTtFKkhbBACpZPsijhfGhXcMVkvb1t1AhvyEKdj7IGHrVFGApfseQfu6iwSfsQbj LwHiYPeVkjgO+2ODZ1czKpDpZL/kqH631WKPSW/eZroPNdqiG8wXAhAGZB7BnNEJplIoT86zO4p 0ZKBaz6uPk5gPOOOcFPKzpbpsDIY74DetQQCtOBUhEDfw/93BupfVcx39Kq+7czkKO5k4APnSiO VsQtObg+NH/xBejnEHQ8TzqE3IImLXAj35CMHKuuIwLnB3Aqp0JDfFL7Mvp7UsHGbVW/dGwB2GX C4LTsCseyg0MWkv6Wh6q9i7GnFQtGTvDgN8UHcPHt9VUPuskRiTa6AWhmfi1RL9uhZIYy12udNJ ECKsvcfQqeY27aiN1FCvi41r3aXgMywIdF5iWkBajTXkge2WQIaVkFIrQFhu5AwwdtqmuEXYXjT xEWkwAnaU35S75/Nw4+Zz1lITHxnhXatwUpYcfwrt9Pe3onnn6e9LB8NtoJfgnJH5vm2+gwDXXW 4OEuPA4dHSgh1mNbkhBSA6T4S20eCuY7dxhHhri89aONG8iKmXSofv/sdGODm+kMwSO6DaqxS46 QLiyZ2g24SkKmLWkx2IIgqt77tuaHaR61e4IR8G0UNgaZpYqmRKFhwukYf0fTpbqvC281QuVMct APCgtEeHflL4ZdKr9Vaa04yzrEd5ZokMflaoNZjQijgrFvzocDDLReGt4PfmUDxpFogQXo8WMkQ Wv6iUD0yuKrQIMCAGLeSok4rrZiYZyrULQL1Yj80Za3RRg8HtvED+8Abgvn+lz6ooB0YJM1x33d 0Fa5rE3kasiloold0xX2Ii4JpA= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--30.718300-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24532.003 X-MDID: 1554465301-C18OF-OiBzlW Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header fields 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: , X-List-Received-Date: Fri, 05 Apr 2019 11:55:02 -0000 On 4/4/19 4:25 PM, Adrien Mazarguil wrote: > Hi Ori, > > (trimming message down a bit) > > On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote: >> Hi Adrien, >> >> PSB > >>> From: Adrien Mazarguil > >>> On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote: >>>> Thanks, PSB. > >>>>> From: Adrien Mazarguil > >>>>> I still don't agree with the wording as it implies one must combine this >>> action >>>>> with the TCP pattern item or else, while one should simply ensure the >>>>> presence of TCP traffic somehow. This may be done by a prior filtering rule. >>>>> >>>>> So here's a generic suggestion which could be used with pretty much all >>>>> modifying actions (other actions have the same problem and will have to be >>>>> fixed as well eventually): >>>>> >>>>> Using this action on non-matching traffic results in undefined behavior. >>>>> >>>>> This comment applies to all instances in this patch. >>>> I accept your suggestion, indeed the existing actions have the problematic >>> condition. >>>> However I would like to currently leave this patch as-is for consistency. >>>> I will send a fix patch for next release, applying the updated text to all >>> modify-header actions. >>> >>> Please do it now as it's much more difficult to change an existing API >>> later (think deprecation notices and endless discussions); even seemingly >>> minor documentation issues like this one may affect applications. >>> >> I agree that changing API is not easy. This is why I think we should keep Dekel patch, >> there is a number of API and consistency is very important. Also the PMD is based on the current >> description that such command should fail. >> >> So lets keep it this way if you want to change all API then and only then this API should be changed. > Wait, I'm not asking Delek to modify existing code/APIs right now, only to > document these new actions properly from the start so we don't have to do it > later (you even acknowledged it's more difficult that way). > > So I fail to understand why it's so important for their documentation to be > consistent with unrelated and badly documented actions? > > Note the change I'm asking for at the API level doesn't affect PMD code, > which remains free to put extra limitations (namely the presence of TCP > pattern items). It's just that these limitations have nothing to do in the > API itself. > > >>>> It's either 2 actions with 1 parameters, or 1 action with 2 parameters. >>>> The current implementation is more straight-forward in my opinion. >>> I generally also prefer the one action per thing to do approach, but seeing >>> the kind of actions you're adding, I fear we'll soon end up with lots of >>> similar rte_flow_action_* structures modifying a single 32-bit value in some >>> way. >>> >>> So for the same reasons as above, I think it's the right time to define a >>> shared structure to rule them all, or maybe even let users provide a >>> rte_be32_t/uint32_t/whatever pointer directly as a conf pointer (not >>> as straightforward to document though). >>> >>> An object to rule them all would look something like that: >>> >>> union rte_flow_integer { >>> rte_be64_t be64; >>> rte_le64_t le64; >>> uint64_t u64; >>> int64_t i64; >>> rte_be32_t be32; >>> rte_le32_t le32; >>> uint32_t u32; >>> int32_t i32; >>> uint8_t u8; >>> int8_t i8; >>> }; >>> >>> Then actions that need a single integer value only have to document which >>> field is relevant to them. How about that? I like the idea (plus 16-bit options). We already have too many rte_flow_action_* structures with single integer in it. >> Like my previous comment. I understand your idea, but it has no huge advantage compared to the >> suggested one by Dekel which also match all other API. >> >> Currently for each action we have a direct command, which is easy to understand by using your idea we break this concept. > Yes, although not all actions have a configuration structure. Those that do > indeed have a rte_flow_action_* counterpart, but it doesn't have to be > unique, see RTE_FLOW_ITEM_GTP/GTPC/GTPU for instance. > > Likewise this patch adds struct rte_flow_action_modify_tcp_seq shared by > RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ and RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ > although they lack a common prefix (inc_tcp/dec_tcp vs. modify_tcp). The > type to use is covered by documentation and that's fine. > > So why not go a little further and share the exact same structure with > RTE_FLOW_ACTION_TYPE_INC_TCP_ACK and RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK? Shouldn't these action be RTE_FLOW_ACTION_TYPE_MOD_TCP_{ACK,SEQ} with singed 32-bit integer parameter (negative to decrement, positive to increment)? IMHO, having INC and DEC is too artificial and just bloat API and code. > And while there, why not plan for subsequent actions that take a single > integer value of some kind, because modifying existing APIs once upstream is > complicated... See where I'm going? > >> There is no issue with having a large number of actions, it is even easer to read and document if each action is dedicated, >> as you can also see from OVS. > I'm actually fine with a large number of actions (rte_flow can support 2^31 > unique actions). Not so much with a large number of identical configuration > structures that only differ by name associated with them. This is what I'd > like to avoid before it's too late. Too many actions bloat the code everywhere: API, PMDs, testpmd, other apps. If it is possible to decrease number of different actions without over-complicating, it should be done. >> So I vote to keep Dekel patch as is. > I don't, I guess another vote is needed to decide :) > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 76A05A0679 for ; Fri, 5 Apr 2019 13:55:06 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D7EAC1B4C1; Fri, 5 Apr 2019 13:55:03 +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 0B7BA1B4C0 for ; Fri, 5 Apr 2019 13:55: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 (Proofpoint Essentials ESMTP Server) with ESMTPS id F3F3B280053; Fri, 5 Apr 2019 11:54:59 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 5 Apr 2019 12:54:53 +0100 To: Adrien Mazarguil , Ori Kam CC: Dekel Peled , "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "dev@dpdk.org" References: <1553177917-43297-1-git-send-email-dekelp@mellanox.com> <1554218001-62012-2-git-send-email-dekelp@mellanox.com> <20190403091432.GP4889@6wind.com> <20190403124921.GR4889@6wind.com> <20190404132556.GS4889@6wind.com> From: Andrew Rybchenko Message-ID: <55805412-3145-4542-9376-67d02a62fa8b@solarflare.com> Date: Fri, 5 Apr 2019 14:54:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <20190404132556.GS4889@6wind.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] 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-24532.003 X-TM-AS-Result: No-30.718300-8.000000-10 X-TMASE-MatchedRID: B0+RG8xpjtQOwH4pD14DsPHkpkyUphL9gHzgwy8qV5p77G36GYsVE/vt 4ENurW24d3NPTtFKkhbBACpZPsijhfGhXcMVkvb1t1AhvyEKdj7IGHrVFGApfseQfu6iwSfsQbj LwHiYPeVkjgO+2ODZ1czKpDpZL/kqH631WKPSW/eZroPNdqiG8wXAhAGZB7BnNEJplIoT86zO4p 0ZKBaz6uPk5gPOOOcFPKzpbpsDIY74DetQQCtOBUhEDfw/93BupfVcx39Kq+7czkKO5k4APnSiO VsQtObg+NH/xBejnEHQ8TzqE3IImLXAj35CMHKuuIwLnB3Aqp0JDfFL7Mvp7UsHGbVW/dGwB2GX C4LTsCseyg0MWkv6Wh6q9i7GnFQtGTvDgN8UHcPHt9VUPuskRiTa6AWhmfi1RL9uhZIYy12udNJ ECKsvcfQqeY27aiN1FCvi41r3aXgMywIdF5iWkBajTXkge2WQIaVkFIrQFhu5AwwdtqmuEXYXjT xEWkwAnaU35S75/Nw4+Zz1lITHxnhXatwUpYcfwrt9Pe3onnn6e9LB8NtoJfgnJH5vm2+gwDXXW 4OEuPA4dHSgh1mNbkhBSA6T4S20eCuY7dxhHhri89aONG8iKmXSofv/sdGODm+kMwSO6DaqxS46 QLiyZ2g24SkKmLWkx2IIgqt77tuaHaR61e4IR8G0UNgaZpYqmRKFhwukYf0fTpbqvC281QuVMct APCgtEeHflL4ZdKr9Vaa04yzrEd5ZokMflaoNZjQijgrFvzocDDLReGt4PfmUDxpFogQXo8WMkQ Wv6iUD0yuKrQIMCAGLeSok4rrZiYZyrULQL1Yj80Za3RRg8HtvED+8Abgvn+lz6ooB0YJM1x33d 0Fa5rE3kasiloold0xX2Ii4JpA= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--30.718300-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24532.003 X-MDID: 1554465301-C18OF-OiBzlW Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header fields 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" Message-ID: <20190405115449.lqgZxb-e9m3Xp7-hrhu5OgxnUyynpejgNUrEh5gzXGw@z> On 4/4/19 4:25 PM, Adrien Mazarguil wrote: > Hi Ori, > > (trimming message down a bit) > > On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote: >> Hi Adrien, >> >> PSB > >>> From: Adrien Mazarguil > >>> On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote: >>>> Thanks, PSB. > >>>>> From: Adrien Mazarguil > >>>>> I still don't agree with the wording as it implies one must combine this >>> action >>>>> with the TCP pattern item or else, while one should simply ensure the >>>>> presence of TCP traffic somehow. This may be done by a prior filtering rule. >>>>> >>>>> So here's a generic suggestion which could be used with pretty much all >>>>> modifying actions (other actions have the same problem and will have to be >>>>> fixed as well eventually): >>>>> >>>>> Using this action on non-matching traffic results in undefined behavior. >>>>> >>>>> This comment applies to all instances in this patch. >>>> I accept your suggestion, indeed the existing actions have the problematic >>> condition. >>>> However I would like to currently leave this patch as-is for consistency. >>>> I will send a fix patch for next release, applying the updated text to all >>> modify-header actions. >>> >>> Please do it now as it's much more difficult to change an existing API >>> later (think deprecation notices and endless discussions); even seemingly >>> minor documentation issues like this one may affect applications. >>> >> I agree that changing API is not easy. This is why I think we should keep Dekel patch, >> there is a number of API and consistency is very important. Also the PMD is based on the current >> description that such command should fail. >> >> So lets keep it this way if you want to change all API then and only then this API should be changed. > Wait, I'm not asking Delek to modify existing code/APIs right now, only to > document these new actions properly from the start so we don't have to do it > later (you even acknowledged it's more difficult that way). > > So I fail to understand why it's so important for their documentation to be > consistent with unrelated and badly documented actions? > > Note the change I'm asking for at the API level doesn't affect PMD code, > which remains free to put extra limitations (namely the presence of TCP > pattern items). It's just that these limitations have nothing to do in the > API itself. > > >>>> It's either 2 actions with 1 parameters, or 1 action with 2 parameters. >>>> The current implementation is more straight-forward in my opinion. >>> I generally also prefer the one action per thing to do approach, but seeing >>> the kind of actions you're adding, I fear we'll soon end up with lots of >>> similar rte_flow_action_* structures modifying a single 32-bit value in some >>> way. >>> >>> So for the same reasons as above, I think it's the right time to define a >>> shared structure to rule them all, or maybe even let users provide a >>> rte_be32_t/uint32_t/whatever pointer directly as a conf pointer (not >>> as straightforward to document though). >>> >>> An object to rule them all would look something like that: >>> >>> union rte_flow_integer { >>> rte_be64_t be64; >>> rte_le64_t le64; >>> uint64_t u64; >>> int64_t i64; >>> rte_be32_t be32; >>> rte_le32_t le32; >>> uint32_t u32; >>> int32_t i32; >>> uint8_t u8; >>> int8_t i8; >>> }; >>> >>> Then actions that need a single integer value only have to document which >>> field is relevant to them. How about that? I like the idea (plus 16-bit options). We already have too many rte_flow_action_* structures with single integer in it. >> Like my previous comment. I understand your idea, but it has no huge advantage compared to the >> suggested one by Dekel which also match all other API. >> >> Currently for each action we have a direct command, which is easy to understand by using your idea we break this concept. > Yes, although not all actions have a configuration structure. Those that do > indeed have a rte_flow_action_* counterpart, but it doesn't have to be > unique, see RTE_FLOW_ITEM_GTP/GTPC/GTPU for instance. > > Likewise this patch adds struct rte_flow_action_modify_tcp_seq shared by > RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ and RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ > although they lack a common prefix (inc_tcp/dec_tcp vs. modify_tcp). The > type to use is covered by documentation and that's fine. > > So why not go a little further and share the exact same structure with > RTE_FLOW_ACTION_TYPE_INC_TCP_ACK and RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK? Shouldn't these action be RTE_FLOW_ACTION_TYPE_MOD_TCP_{ACK,SEQ} with singed 32-bit integer parameter (negative to decrement, positive to increment)? IMHO, having INC and DEC is too artificial and just bloat API and code. > And while there, why not plan for subsequent actions that take a single > integer value of some kind, because modifying existing APIs once upstream is > complicated... See where I'm going? > >> There is no issue with having a large number of actions, it is even easer to read and document if each action is dedicated, >> as you can also see from OVS. > I'm actually fine with a large number of actions (rte_flow can support 2^31 > unique actions). Not so much with a large number of identical configuration > structures that only differ by name associated with them. This is what I'd > like to avoid before it's too late. Too many actions bloat the code everywhere: API, PMDs, testpmd, other apps. If it is possible to decrease number of different actions without over-complicating, it should be done. >> So I vote to keep Dekel patch as is. > I don't, I guess another vote is needed to decide :) >