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 78F13A05D3 for ; Mon, 22 Apr 2019 09:15:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4131C1B4B3; Mon, 22 Apr 2019 09:15:19 +0200 (CEST) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-eopbgr140045.outbound.protection.outlook.com [40.107.14.45]) by dpdk.org (Postfix) with ESMTP id B35AF1B3A4 for ; Mon, 22 Apr 2019 09:15:17 +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=KJ35xO+WWYQAtBiAjATdkzDjTVcX9gCQVUJCmM87qrw=; b=R6fAfFrM36rt5pSXbaCC99ZxGvxthJC3GPGpZnjPGZfpa/o66yFXzqr9dvmxkKY21EFveNXpzx4KgSx36GcWCUieAbA4LeL7aeJgCp3qieyCqFY2OXC6fVXtYzYgazc6Horw39lVoIRSmy3BcxuqqDsfPj5cVAzLKC+0OMlcgO0= Received: from VI1PR05MB4224.eurprd05.prod.outlook.com (52.133.12.13) by VI1PR05MB3246.eurprd05.prod.outlook.com (10.170.238.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1813.12; Mon, 22 Apr 2019 07:15:15 +0000 Received: from VI1PR05MB4224.eurprd05.prod.outlook.com ([fe80::a511:20b2:5fb2:3a5d]) by VI1PR05MB4224.eurprd05.prod.outlook.com ([fe80::a511:20b2:5fb2:3a5d%2]) with mapi id 15.20.1813.017; Mon, 22 Apr 2019 07:15:15 +0000 From: Dekel Peled To: Adrien Mazarguil CC: "wenzhuo.lu@intel.com" , "jingjing.wu@intel.com" , "bernard.iremonger@intel.com" , Yongseok Koh , Shahaf Shuler , "arybchenko@solarflare.com" , "dev@dpdk.org" , Ori Kam Thread-Topic: [PATCH v4 1/3] ethdev: add actions to modify TCP header fields Thread-Index: AQHU75Pw0wIStfkoUE2yx2lO/nKDyaZB5fWAgAXjrvA= Date: Mon, 22 Apr 2019 07:15:15 +0000 Message-ID: References: <20190418123051.GD4889@6wind.com> In-Reply-To: <20190418123051.GD4889@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=dekelp@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: f43e6f11-3e12-436d-3f8d-08d6c6f244b0 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600141)(711020)(4605104)(4618075)(2017052603328)(7193020); SRVR:VI1PR05MB3246; x-ms-traffictypediagnostic: VI1PR05MB3246: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-forefront-prvs: 00159D1518 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(366004)(376002)(346002)(136003)(39860400002)(13464003)(189003)(199004)(229853002)(7736002)(6916009)(4326008)(76176011)(93886005)(25786009)(33656002)(68736007)(74316002)(305945005)(316002)(7696005)(86362001)(478600001)(54906003)(66066001)(99286004)(14454004)(8936002)(81156014)(8676002)(5660300002)(81166006)(71190400001)(6436002)(71200400001)(52536014)(2906002)(97736004)(11346002)(446003)(53936002)(486006)(66476007)(66556008)(64756008)(66446008)(476003)(9686003)(26005)(55016002)(53546011)(6506007)(102836004)(3846002)(6116002)(6246003)(66946007)(186003)(107886003)(14444005)(256004)(76116006)(73956011); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR05MB3246; H:VI1PR05MB4224.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: V4yii0e2wnQxlIy7bdgJWUhiMKxjDweD2VhrrI2BP+mqoDUVLnRSBxRxlN9YtFEeoJm7nAQgV9kYpeG/OuA1eS0LfwqrlB7RKO/bF3lGmjq+zzGTFk8Iav4w6aCFYe+ukinyRPodYrafigtqBxp+818H3fAlQOAKwsd+0HI/+I9bOa//1VLWk30o28/cgacYetmN+amE0XSXfKfYVoJ0a6Jpgq/or7zZ3RfVqD396jjXZY1HU/pa5EVH/jb/qt0I3qvbaGEv8HeMF2Yi3eBqGIXKZ+152h3KfIZB8vNwUhfDjRNp2y5Fz2hJyUS6asH40QAv3qAuLPhtLVsHIPt5EC8rX3E9vNgQ+BluOkM5kPJZYCIld1jZ9NxhJ5R0a/mBphQl/PHR0kxFZVO1cGdcp6JwHpKW5VGhGnCE+QyXSh4= 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: f43e6f11-3e12-436d-3f8d-08d6c6f244b0 X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2019 07:15:15.3658 (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: VI1PR05MB3246 Subject: Re: [dpdk-dev] [PATCH v4 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: <20190422071515.q8GZodrgPmgWChDRwnFr_oZUoaiwXLz9R2Xzr-JUs-0@z> Thanks, PSB. > -----Original Message----- > From: Adrien Mazarguil > Sent: Thursday, April 18, 2019 3:31 PM > To: Dekel Peled > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com; > bernard.iremonger@intel.com; Yongseok Koh ; > Shahaf Shuler ; arybchenko@solarflare.com; > dev@dpdk.org; Ori Kam > Subject: Re: [PATCH v4 1/3] ethdev: add actions to modify TCP header fiel= ds >=20 > On Wed, Apr 10, 2019 at 02:50:41PM +0300, Dekel Peled wrote: > > Add actions: > > - INC_TCP_SEQ - Increase sequence number in the outermost TCP header. > > - 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 >=20 > Almost there... Some changes were not made as discussed, please see > below. >=20 > > > diff --git a/doc/guides/prog_guide/rte_flow.rst > > b/doc/guides/prog_guide/rte_flow.rst > > index 0203f4f..fc234de 100644 > > --- a/doc/guides/prog_guide/rte_flow.rst > > +++ b/doc/guides/prog_guide/rte_flow.rst > > @@ -2345,6 +2345,78 @@ Otherwise, RTE_FLOW_ERROR_TYPE_ACTION > error will be returned. > > | ``mac_addr`` | MAC address | > > +--------------+---------------+ > > > > +Action: ``INC_TCP_SEQ`` > > +^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Increase sequence number in the outermost TCP header. > > + > > +Using this action on non-matching traffic will result in undefined > > +behavior, depending on PMD implementation. >=20 > OK, "depending on PMD implementation" looks a bit redundant though. Fine, will remove it. >=20 > > + > > +.. _table_rte_flow_action_inc_tcp_seq: > > + > > +.. table:: INC_TCP_SEQ > > + > > + +-----------+------------------------------------------+ > > + | Field | Value | > > + > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D+ > > + | ``value`` | Value to increase TCP sequence number by | > > + +-----------+------------------------------------------+ >=20 > Configuration object documentation needs updating since you changed its > type and fields. >=20 > These comments also apply to DEC_TCP_SEQ, INC_TCP_ACK and > DEC_TCP_ACK. Will update. >=20 > > > diff --git a/lib/librte_ethdev/rte_flow.h > > b/lib/librte_ethdev/rte_flow.h index c0fe879..e3f6210 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -1651,6 +1651,46 @@ enum rte_flow_action_type { > > * See struct rte_flow_action_set_mac. > > */ > > RTE_FLOW_ACTION_TYPE_SET_MAC_DST, > > + > > + /** > > + * Increase sequence number in the outermost TCP header. > > + * > > + * Using this action on non-matching traffic will result in > > + * undefined behavior, depending on PMD implementation. >=20 > "depending on PMD implementation" also redundant here. >=20 > > > /* > > + * @warning > > + * @b EXPERIMENTAL: this union may change without prior notice > > + * > > + * General integer type, can be expanded as needed. > > + */ > > +union rte_flow_integer { > > + rte_be32_t be32; > > +}; >=20 > You must include the extra fields you don't use (64/16/32/8 and > le/be/host/signed variants as described in previous messages) to limit th= e > risk of ABI breakage next time someone needs a field that isn't there yet= . >=20 > This object must be able to accommodate the largest supported integer and > have the proper alignment constraints for any of these types, hence the > union with all of them from the start. I will add required fields for 8, 16 and 32 bits. Adding 64 bit fields will inflate the union size, wasting memory. It should be handled separately in specific cases. >=20 > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > + * > > + * General struct, for use by actions that require a single integer va= lue. > > + */ > > +struct rte_flow_general_action { > > + union rte_flow_integer integer; > > +}; >=20 > Seriously, why can't actions take "union rte_flow_integer" directly? Besi= des > "rte_flow_general_action" is vague as heck. >=20 I prefer to follow the existing convention, where actions take struct even = if it contains a single field. I will define the union unnamed in the struct. I will rename to rte_flow_integer_action. > Seems like you made this structure so it could be extended later, but for= got > that doing so is not an option since ABIs are set in stone. You must make= new > APIs as exhaustive and restrict their scope as much as possible from the = start > because of that. >=20 > Note if you don't want to use union rte_flow_integer directly, it's also = fine to > document your actions as taking (const rte_be32_t *) directly through the= ir > conf pointer. >=20 > -- > Adrien Mazarguil > 6WIND