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 25846A0679 for ; Thu, 4 Apr 2019 11:01:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8AB0E5B40; Thu, 4 Apr 2019 11:01:55 +0200 (CEST) Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00058.outbound.protection.outlook.com [40.107.0.58]) by dpdk.org (Postfix) with ESMTP id 1937B56A1 for ; Thu, 4 Apr 2019 11:01:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=htktjtC0hspp1+N1M3minQuG87nv+Js36u0VPUiMFd4=; b=yi+vgJAba1Qjm7qJEa4SOYODI5CbScdFGCmhy3d2RQb3VDFARuZLU+3joXnrMsx2gCh9yFc5P2rFALKj6sE3tt8NsiN6A383C5nBOP91f50q89uC069B8YOOPu5oQwA9Famk6KQFPl+C34Qht8r0n70IuswqQI4XfHeIQy5O8Zg= Received: from AM4PR05MB3425.eurprd05.prod.outlook.com (10.171.190.15) by AM4PR05MB3187.eurprd05.prod.outlook.com (10.171.186.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1750.16; Thu, 4 Apr 2019 09:01:52 +0000 Received: from AM4PR05MB3425.eurprd05.prod.outlook.com ([fe80::5df0:22de:97f0:3827]) by AM4PR05MB3425.eurprd05.prod.outlook.com ([fe80::5df0:22de:97f0:3827%4]) with mapi id 15.20.1771.014; Thu, 4 Apr 2019 09:01:52 +0000 From: Ori Kam To: Adrien Mazarguil , Dekel Peled CC: "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields Thread-Index: AQHU6f2osUU8ZA0a0EGAi4+KAkLQbqYqQcCAgAAhlYCAAU+ZkA== Date: Thu, 4 Apr 2019 09:01:52 +0000 Message-ID: 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> In-Reply-To: <20190403124921.GR4889@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=orika@mellanox.com; x-originating-ip: [185.175.35.255] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 774b7e11-a5f1-49a8-334c-08d6b8dc2e2d x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(5600139)(711020)(4605104)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020); SRVR:AM4PR05MB3187; x-ms-traffictypediagnostic: AM4PR05MB3187: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-forefront-prvs: 0997523C40 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(136003)(346002)(366004)(376002)(39860400002)(189003)(199004)(13464003)(106356001)(6636002)(6116002)(11346002)(446003)(476003)(2906002)(99286004)(55016002)(5660300002)(105586002)(81156014)(6246003)(486006)(25786009)(7736002)(54906003)(186003)(26005)(74316002)(6506007)(86362001)(110136005)(478600001)(53936002)(14444005)(71190400001)(7696005)(4326008)(6436002)(305945005)(33656002)(52536014)(53546011)(14454004)(102836004)(71200400001)(3846002)(81166006)(76176011)(316002)(97736004)(68736007)(9686003)(93886005)(66066001)(229853002)(256004)(8936002)(8676002)(7756004); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3187; H:AM4PR05MB3425.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: bIMWcMRHinDesLh3hC4zQ+P8wW02AStMK02sKnpA7WBcKQtMeBxvGllXAHiQGV4dSudJut/Q0UlTaolY6sEg1prh8rTOnVvG+ngNHo8mydirtKhFCPvI9hRslVSEOCc4BmX1ViYHLOvDwv9/bGS45CIVswe1k5sRflDSICd1jT/QO/fKsGyglK28TODoFFgnw4nO/ht2uD4DX6QLls3jZVV5nMDA/chhIvGmypdy6wXua9qiVa/wwAGScYqn53XOoONprhNm7SRrYfsgxqz/7V/HiPnvNpoDiOdaMQW8LXntHnmV1u7swxkFDTVz4WU708a8CJY5UfMX2/oqtSR7w3XxVq5kLttFoaUhEt7OsGpcg+HsjBXo6B5QWgWiCrH/89oSreVPtd4XKb6I1dYZpH8eNVJ9AA9n9m9DfnM7GWI= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 774b7e11-a5f1-49a8-334c-08d6b8dc2e2d X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Apr 2019 09:01:52.5183 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3187 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: <20190404090152.W4EWmX4P5qC7Fp1YVrolMvbP8nQfF2nzoZRYyBKCSGY@z> Hi Adrien, PSB > -----Original Message----- > From: Adrien Mazarguil > Sent: Wednesday, April 3, 2019 3:49 PM > To: Dekel Peled > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com; > bernard.iremonger@intel.com; Yongseok Koh ; Shahaf > Shuler ; dev@dpdk.org; Ori Kam > > Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fiel= ds >=20 > On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote: > > Thanks, PSB. > > > > > -----Original Message----- > > > From: Adrien Mazarguil > > > Sent: Wednesday, April 3, 2019 12:15 PM > > > To: Dekel Peled > > > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com; > > > bernard.iremonger@intel.com; Yongseok Koh ; > > > Shahaf Shuler ; dev@dpdk.org; Ori Kam > > > > > > Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header = fields > > > > > > Hi Dekel, > > > > > > On Tue, Apr 02, 2019 at 06:13:19PM +0300, Dekel Peled wrote: > > > > Add actions: > > > > - INC_TCP_SEQ - Increase sequence number in the outermost TCP heade= r. > > > > - DEC_TCP_SEQ - Decrease sequence number in the outermost TCP > > > header. > > > > - INC_TCP_ACK - Increase acknowledgment number in the outermost TCP > > > > header. > > > > - DEC_TCP_ACK - Decrease acknowledgment number in the outermost > TCP > > > > header. > > > > > > > > Original work by Xiaoyu Min. > > > > > > > > Signed-off-by: Dekel Peled > > > > > > > +Action: ``INC_TCP_SEQ`` > > > > +^^^^^^^^^^^^^^^^^^^^^^^ > > > > + > > > > +Increase sequence number in the outermost TCP header. > > > > + > > > > +If this action is used without a valid RTE_FLOW_ITEM_TYPE_TCP flow > > > > +pattern item, behavior is unspecified, depending on PMD > > > implementation. > > > > > > I still don't agree with the wording as it implies one must combine t= his > 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 filterin= g rule. > > > > > > So here's a generic suggestion which could be used with pretty much a= ll > > > 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 behav= ior. > > > > > > This comment applies to all instances in this patch. > > > > I accept your suggestion, indeed the existing actions have the problema= tic > condition. > > However I would like to currently leave this patch as-is for consistenc= y. > > I will send a fix patch for next release, applying the updated text to = all > modify-header actions. >=20 > 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. >=20 I agree that changing API is not easy. This is why I think we should keep D= ekel 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 t= his API should be changed. > > > > > > > +/** > > > > + * @warning > > > > + * @b EXPERIMENTAL: this structure may change without prior notice > > > > + * > > > > + * RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ > > > > + * RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ > > > > + * > > > > + * Increase/Decrease outermost TCP sequence number */ struct > > > > +rte_flow_action_modify_tcp_seq { > > > > + rte_be32_t value; /**< Value to increase/decrease by. */ }; > > > > + > > > > +/** > > > > + * @warning > > > > + * @b EXPERIMENTAL: this structure may change without prior notice > > > > + * > > > > + * RTE_FLOW_ACTION_TYPE_INC_TCP_ACK > > > > + * RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK > > > > + * > > > > + * Increase/Decrease outermost TCP acknowledgment number. > > > > + */ > > > > +struct rte_flow_action_modify_tcp_ack { > > > > + rte_be32_t value; /**< Value to increase/decrease by. */ }; > > > > > > Thanks for adding experimental tags and comments, however you didn't > > > reply anything about using a single action, or at least a single stru= cture for > > > add/sub/set? I'd like to hear your thoughts. > > > > It's either 2 actions with 1 parameters, or 1 action with 2 parameters. > > The current implementation is more straight-forward in my opinion. >=20 > I generally also prefer the one action per thing to do approach, but seei= ng > 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 s= ome > way. >=20 > 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). >=20 > An object to rule them all would look something like that: >=20 > 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; > }; >=20 > Then actions that need a single integer value only have to document which > field is relevant to them. How about that? >=20 Like my previous comment. I understand your idea, but it has no huge advant= age 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 unders= tand by using your idea we break this concept. There is no issue with having a large number of actions, it is even easer t= o read and document if each action is dedicated, as you can also see from OVS. So I vote to keep Dekel patch as is. > -- > Adrien Mazarguil > 6WIND Ori Kam