From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ori Kam <orika@mellanox.com>
Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP
	header fields
Date: Thu, 4 Apr 2019 09:01:52 +0000
Message-ID: <AM4PR05MB342559B6EA39BF745EC3690DDB500@AM4PR05MB3425.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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
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>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>, Dekel Peled
 <dekelp@mellanox.com>
Return-path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Thu,  4 Apr 2019 11:01:54 +0200 (CEST)
In-Reply-To: <20190403124921.GR4889@6wind.com>
Content-Language: en-US
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>

Hi Adrien,

PSB

> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 3, 2019 3:49 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>; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>
> 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 <adrien.mazarguil@6wind.com>
> > > Sent: Wednesday, April 3, 2019 12:15 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>; dev@dpdk.org; Ori Kam
> > > <orika@mellanox.com>
> > > 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 <dekelp@mellanox.com>
> > > <snip>
> > > > +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.

> > > <snip>
> > > > +/**
> > > > + * @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