From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dekelp@mellanox.com>
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 <dev@dpdk.org>; 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 <dekelp@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
CC: "wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>, "jingjing.wu@intel.com"
 <jingjing.wu@intel.com>, "bernard.iremonger@intel.com"
 <bernard.iremonger@intel.com>, Yongseok Koh <yskoh@mellanox.com>, Shahaf
 Shuler <shahafs@mellanox.com>, "arybchenko@solarflare.com"
 <arybchenko@solarflare.com>, "dev@dpdk.org" <dev@dpdk.org>, Ori Kam
 <orika@mellanox.com>
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: <VI1PR05MB42244A2AD2478C010E165CAEB6220@VI1PR05MB4224.eurprd05.prod.outlook.com>
References: <cover.1554894982.git.dekelp@mellanox.com>
 <cover.1554896841.git.dekelp@mellanox.com>
 <d0206424d8e432ecb1bdc339705e488b3c7c63a5.1554896841.git.dekelp@mellanox.com>
 <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: <VI1PR05MB3246ED384B9CB060273D9F23B6220@VI1PR05MB3246.eurprd05.prod.outlook.com>
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="us-ascii"
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 22 Apr 2019 07:15:17 -0000

Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, April 18, 2019 3:31 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; Yongseok Koh <yskoh@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; arybchenko@solarflare.com;
> dev@dpdk.org; Ori Kam <orika@mellanox.com>
> 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 <dekelp@mellanox.com>
>=20
> Almost there... Some changes were not made as discussed, please see
> below.
>=20
> <snip>
> > 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
> <snip>
> > 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
> <snip>
> >  /*
> > + * @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

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by dpdk.space (Postfix) with ESMTP id 78F13A05D3
	for <public@inbox.dpdk.org>; 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 <dev@dpdk.org>; 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 <dekelp@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
CC: "wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>, "jingjing.wu@intel.com"
 <jingjing.wu@intel.com>, "bernard.iremonger@intel.com"
 <bernard.iremonger@intel.com>, Yongseok Koh <yskoh@mellanox.com>, Shahaf
 Shuler <shahafs@mellanox.com>, "arybchenko@solarflare.com"
 <arybchenko@solarflare.com>, "dev@dpdk.org" <dev@dpdk.org>, Ori Kam
 <orika@mellanox.com>
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:
 <VI1PR05MB42244A2AD2478C010E165CAEB6220@VI1PR05MB4224.eurprd05.prod.outlook.com>
References: <cover.1554894982.git.dekelp@mellanox.com>
 <cover.1554896841.git.dekelp@mellanox.com>
 <d0206424d8e432ecb1bdc339705e488b3c7c63a5.1554896841.git.dekelp@mellanox.com>
 <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: <VI1PR05MB3246ED384B9CB060273D9F23B6220@VI1PR05MB3246.eurprd05.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190422071515.q8GZodrgPmgWChDRwnFr_oZUoaiwXLz9R2Xzr-JUs-0@z>

Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, April 18, 2019 3:31 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; Yongseok Koh <yskoh@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; arybchenko@solarflare.com;
> dev@dpdk.org; Ori Kam <orika@mellanox.com>
> 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 <dekelp@mellanox.com>
>=20
> Almost there... Some changes were not made as discussed, please see
> below.
>=20
> <snip>
> > 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
> <snip>
> > 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
> <snip>
> >  /*
> > + * @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