From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dekelp@mellanox.com>
Received: from EUR04-VI1-obe.outbound.protection.outlook.com
 (mail-eopbgr80045.outbound.protection.outlook.com [40.107.8.45])
 by dpdk.org (Postfix) with ESMTP id 641204CA6
 for <dev@dpdk.org>; Mon,  8 Apr 2019 15:36:57 +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=+8r6YXZeczAFlkAz7E1nWWAgkfKFV8Aruc/a+n2Akns=;
 b=JFpRXWA/TP5w5UHrQZVDa1uJN3YbrxAWaO7rDT2CSsp3UJZqMtzk4k8UWsjNicNd6E/MPI3LrbiYboS28Fr+/imf+jTLeKWNBK2JWflCBHyBnKLrga4fKboLWWfJfxL+6ooUmz/MXnwLEX21x3AnkoIQE8oN9P0JGYaNmE8TlwU=
Received: from VI1PR05MB4224.eurprd05.prod.outlook.com (52.133.12.13) by
 VI1PR05MB6319.eurprd05.prod.outlook.com (20.179.25.23) with Microsoft SMTP
 Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.20.1771.16; Mon, 8 Apr 2019 13:36:56 +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.1771.021; Mon, 8 Apr 2019
 13:36:55 +0000
From: Dekel Peled <dekelp@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>, Ori Kam
 <orika@mellanox.com>, Andrew Rybchenko <arybchenko@solarflare.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>, "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields
Thread-Index: AQHU6WbqvvsJSYxgskWKjtJdhwgGX6YqKH0AgAAVcrCAACaTgIABUsYAgABJyACABkPfoA==
Date: Mon, 8 Apr 2019 13:36:55 +0000
Message-ID: <VI1PR05MB4224AB74A0DB51996BEFD8EFB62C0@VI1PR05MB4224.eurprd05.prod.outlook.com>
References: <1553177917-43297-1-git-send-email-dekelp@mellanox.com>
 <1554218001-62012-2-git-send-email-dekelp@mellanox.com>
 <20190403091432.GP4889@6wind.com>
 <VI1PR05MB4224753382F523CD7C238A7EB6570@VI1PR05MB4224.eurprd05.prod.outlook.com>
 <20190403124921.GR4889@6wind.com>
 <AM4PR05MB342559B6EA39BF745EC3690DDB500@AM4PR05MB3425.eurprd05.prod.outlook.com>
 <20190404132556.GS4889@6wind.com>
In-Reply-To: <20190404132556.GS4889@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: 60b791fa-bdae-43c4-e86d-08d6bc274496
x-ms-office365-filtering-ht: Tenant
x-microsoft-antispam: BCL:0; PCL:0;
 RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600139)(711020)(4605104)(4618075)(2017052603328)(7193020);
 SRVR:VI1PR05MB6319; 
x-ms-traffictypediagnostic: VI1PR05MB6319:
x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr
x-microsoft-antispam-prvs: <VI1PR05MB6319838CC355A473D6E87A3CB62C0@VI1PR05MB6319.eurprd05.prod.outlook.com>
x-forefront-prvs: 0001227049
x-forefront-antispam-report: SFV:NSPM;
 SFS:(10009020)(396003)(376002)(346002)(39860400002)(366004)(136003)(13464003)(189003)(199004)(5660300002)(26005)(53546011)(76176011)(8936002)(8676002)(81166006)(81156014)(71190400001)(6246003)(71200400001)(3846002)(9686003)(97736004)(105586002)(6506007)(102836004)(86362001)(6116002)(68736007)(66066001)(106356001)(14454004)(55016002)(99286004)(316002)(110136005)(11346002)(93886005)(186003)(7696005)(486006)(54906003)(229853002)(256004)(74316002)(476003)(52536014)(305945005)(7736002)(25786009)(53936002)(14444005)(4326008)(6436002)(446003)(2906002)(478600001)(33656002);
 DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR05MB6319;
 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: p15X+mLqLJxV2cCStb1E3WP+Zagk4RmNYDwoL2OtPdj4vPimSJl688T8XUFuqBjI06fh2B8mzevY1aBdSgFolbluki/9DOV8IVDWS1N0koIsf1p8b+q1HeEHSZk+v492Xzbc4JI+qQYbruseiuwCYt3EMekeATATToWqFFcq9gq9I07TdlXNgBbLU+qB08OcnUZ2HDMWGbszaLEebVR9/jruZt0xJP0LKtIQmv8ic6fDYDMbsdOb6UehBgO0yaiARAmUnXvKwg1kw/sbVvls0i0msWhBGQcI8lhmou6f+tdygqDpbapWsGolbmifPulO3cMmDwc7bsddYWJPgGjronlB1qLJnKLic84lBMuPU9ORb7pBV+UgdHYAdsQZltugjZrpw3PDilBqPwcGf1QpXPn+MpsxRHaLJ+Sxf/80gLI=
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: 60b791fa-bdae-43c4-e86d-08d6bc274496
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2019 13:36:55.9010 (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: VI1PR05MB6319
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 <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, 08 Apr 2019 13:36:57 -0000

Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, April 4, 2019 4:26 PM
> To: Ori Kam <orika@mellanox.com>
> Cc: Dekel Peled <dekelp@mellanox.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fiel=
ds
>=20
> Hi Ori,
>=20
> (trimming message down a bit)
>=20
> On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote:
> > Hi Adrien,
> >
> > PSB
> <snip>
> >
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
> > > On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote:
> > > > Thanks, PSB.
> <snip>
> > > > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
> > > > > 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 consis=
tency.
> > > > 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 th=
en this
> API should be changed.
>=20
> Wait, I'm not asking Delek to modify existing code/APIs right now, only t=
o

It's Dekel, not Delek (nor any other permutation of these letters).

> 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).
>=20
> So I fail to understand why it's so important for their documentation to =
be
> consistent with unrelated and badly documented actions?
>=20
> 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 th=
e API
> itself.

Accepted, I will change the documentation as you suggested, with note that =
the resulting undefined behavior is per PMD implementation.

Regarding Andrew's suggestion: "Shouldn't these action be RTE_FLOW_ACTION_T=
YPE_MOD_TCP_{ACK,SEQ} with singed 32-bit integer parameter (negative to dec=
rement, positive to increment)?"
I will leave the actions as is, the action names indicate the operation the=
y perform.=20

>=20
> <snip>
> > > > It's either 2 actions with 1 parameters, or 1 action with 2 paramet=
ers.
> > > > 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?
> > >
> >
> > 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.
>=20
> 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.
>=20
> 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.
>=20
> 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?
>=20

Accepted, I will add union as you suggested (plus 16 bit values as Andrew n=
oted) and use it for all the new actions.

> 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?
>=20
> > 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.
>=20
> 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 configurati=
on
> structures that only differ by name associated with them. This is what I'=
d like
> to avoid before it's too late.
>=20
> > So I vote to keep Dekel patch as is.
>=20
> I don't, I guess another vote is needed to decide :)
>=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 DB3FEA0096
	for <public@inbox.dpdk.org>; Mon,  8 Apr 2019 15:36:58 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 80EC64CC7;
	Mon,  8 Apr 2019 15:36:58 +0200 (CEST)
Received: from EUR04-VI1-obe.outbound.protection.outlook.com
 (mail-eopbgr80045.outbound.protection.outlook.com [40.107.8.45])
 by dpdk.org (Postfix) with ESMTP id 641204CA6
 for <dev@dpdk.org>; Mon,  8 Apr 2019 15:36:57 +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=+8r6YXZeczAFlkAz7E1nWWAgkfKFV8Aruc/a+n2Akns=;
 b=JFpRXWA/TP5w5UHrQZVDa1uJN3YbrxAWaO7rDT2CSsp3UJZqMtzk4k8UWsjNicNd6E/MPI3LrbiYboS28Fr+/imf+jTLeKWNBK2JWflCBHyBnKLrga4fKboLWWfJfxL+6ooUmz/MXnwLEX21x3AnkoIQE8oN9P0JGYaNmE8TlwU=
Received: from VI1PR05MB4224.eurprd05.prod.outlook.com (52.133.12.13) by
 VI1PR05MB6319.eurprd05.prod.outlook.com (20.179.25.23) with Microsoft SMTP
 Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.20.1771.16; Mon, 8 Apr 2019 13:36:56 +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.1771.021; Mon, 8 Apr 2019
 13:36:55 +0000
From: Dekel Peled <dekelp@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>, Ori Kam
 <orika@mellanox.com>, Andrew Rybchenko <arybchenko@solarflare.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>, "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields
Thread-Index: AQHU6WbqvvsJSYxgskWKjtJdhwgGX6YqKH0AgAAVcrCAACaTgIABUsYAgABJyACABkPfoA==
Date: Mon, 8 Apr 2019 13:36:55 +0000
Message-ID:
 <VI1PR05MB4224AB74A0DB51996BEFD8EFB62C0@VI1PR05MB4224.eurprd05.prod.outlook.com>
References: <1553177917-43297-1-git-send-email-dekelp@mellanox.com>
 <1554218001-62012-2-git-send-email-dekelp@mellanox.com>
 <20190403091432.GP4889@6wind.com>
 <VI1PR05MB4224753382F523CD7C238A7EB6570@VI1PR05MB4224.eurprd05.prod.outlook.com>
 <20190403124921.GR4889@6wind.com>
 <AM4PR05MB342559B6EA39BF745EC3690DDB500@AM4PR05MB3425.eurprd05.prod.outlook.com>
 <20190404132556.GS4889@6wind.com>
In-Reply-To: <20190404132556.GS4889@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: 60b791fa-bdae-43c4-e86d-08d6bc274496
x-ms-office365-filtering-ht: Tenant
x-microsoft-antispam: BCL:0; PCL:0;
 RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600139)(711020)(4605104)(4618075)(2017052603328)(7193020);
 SRVR:VI1PR05MB6319; 
x-ms-traffictypediagnostic: VI1PR05MB6319:
x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr
x-microsoft-antispam-prvs: <VI1PR05MB6319838CC355A473D6E87A3CB62C0@VI1PR05MB6319.eurprd05.prod.outlook.com>
x-forefront-prvs: 0001227049
x-forefront-antispam-report: SFV:NSPM;
 SFS:(10009020)(396003)(376002)(346002)(39860400002)(366004)(136003)(13464003)(189003)(199004)(5660300002)(26005)(53546011)(76176011)(8936002)(8676002)(81166006)(81156014)(71190400001)(6246003)(71200400001)(3846002)(9686003)(97736004)(105586002)(6506007)(102836004)(86362001)(6116002)(68736007)(66066001)(106356001)(14454004)(55016002)(99286004)(316002)(110136005)(11346002)(93886005)(186003)(7696005)(486006)(54906003)(229853002)(256004)(74316002)(476003)(52536014)(305945005)(7736002)(25786009)(53936002)(14444005)(4326008)(6436002)(446003)(2906002)(478600001)(33656002);
 DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR05MB6319;
 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: p15X+mLqLJxV2cCStb1E3WP+Zagk4RmNYDwoL2OtPdj4vPimSJl688T8XUFuqBjI06fh2B8mzevY1aBdSgFolbluki/9DOV8IVDWS1N0koIsf1p8b+q1HeEHSZk+v492Xzbc4JI+qQYbruseiuwCYt3EMekeATATToWqFFcq9gq9I07TdlXNgBbLU+qB08OcnUZ2HDMWGbszaLEebVR9/jruZt0xJP0LKtIQmv8ic6fDYDMbsdOb6UehBgO0yaiARAmUnXvKwg1kw/sbVvls0i0msWhBGQcI8lhmou6f+tdygqDpbapWsGolbmifPulO3cMmDwc7bsddYWJPgGjronlB1qLJnKLic84lBMuPU9ORb7pBV+UgdHYAdsQZltugjZrpw3PDilBqPwcGf1QpXPn+MpsxRHaLJ+Sxf/80gLI=
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: 60b791fa-bdae-43c4-e86d-08d6bc274496
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2019 13:36:55.9010 (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: VI1PR05MB6319
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 <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: <20190408133655.RbFSuAxB0nQPHKu6mT-PAq3t22IG7zmNXGYl9-2l57A@z>

Thanks, PSB.

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, April 4, 2019 4:26 PM
> To: Ori Kam <orika@mellanox.com>
> Cc: Dekel Peled <dekelp@mellanox.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fiel=
ds
>=20
> Hi Ori,
>=20
> (trimming message down a bit)
>=20
> On Thu, Apr 04, 2019 at 09:01:52AM +0000, Ori Kam wrote:
> > Hi Adrien,
> >
> > PSB
> <snip>
> >
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
> > > On Wed, Apr 03, 2019 at 10:49:09AM +0000, Dekel Peled wrote:
> > > > Thanks, PSB.
> <snip>
> > > > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> <snip>
> > > > > 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 consis=
tency.
> > > > 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 th=
en this
> API should be changed.
>=20
> Wait, I'm not asking Delek to modify existing code/APIs right now, only t=
o

It's Dekel, not Delek (nor any other permutation of these letters).

> 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).
>=20
> So I fail to understand why it's so important for their documentation to =
be
> consistent with unrelated and badly documented actions?
>=20
> 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 th=
e API
> itself.

Accepted, I will change the documentation as you suggested, with note that =
the resulting undefined behavior is per PMD implementation.

Regarding Andrew's suggestion: "Shouldn't these action be RTE_FLOW_ACTION_T=
YPE_MOD_TCP_{ACK,SEQ} with singed 32-bit integer parameter (negative to dec=
rement, positive to increment)?"
I will leave the actions as is, the action names indicate the operation the=
y perform.=20

>=20
> <snip>
> > > > It's either 2 actions with 1 parameters, or 1 action with 2 paramet=
ers.
> > > > 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?
> > >
> >
> > 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.
>=20
> 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.
>=20
> 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.
>=20
> 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?
>=20

Accepted, I will add union as you suggested (plus 16 bit values as Andrew n=
oted) and use it for all the new actions.

> 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?
>=20
> > 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.
>=20
> 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 configurati=
on
> structures that only differ by name associated with them. This is what I'=
d like
> to avoid before it's too late.
>=20
> > So I vote to keep Dekel patch as is.
>=20
> I don't, I guess another vote is needed to decide :)
>=20
> --
> Adrien Mazarguil
> 6WIND