DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
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>
Subject: Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header fields
Date: Mon, 8 Apr 2019 13:36:55 +0000
Message-ID: <VI1PR05MB4224AB74A0DB51996BEFD8EFB62C0@VI1PR05MB4224.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20190404132556.GS4889@6wind.com>

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 fields
> 
> Hi Ori,
> 
> (trimming message down a bit)
> 
> 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 consistency.
> > > > 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 then this
> API should be changed.
> 
> Wait, I'm not asking Delek to modify existing code/APIs right now, only to

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).
> 
> So I fail to understand why it's so important for their documentation to be
> consistent with unrelated and badly documented actions?
> 
> 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 the 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_TYPE_MOD_TCP_{ACK,SEQ} with singed 32-bit integer parameter (negative to decrement, positive to increment)?"
I will leave the actions as is, the action names indicate the operation they perform. 

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

Accepted, I will add union as you suggested (plus 16 bit values as Andrew noted) 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?
> 
> > 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.
> 
> 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 configuration
> structures that only differ by name associated with them. This is what I'd like
> to avoid before it's too late.
> 
> > So I vote to keep Dekel patch as is.
> 
> I don't, I guess another vote is needed to decide :)
> 
> --
> Adrien Mazarguil
> 6WIND

  parent reply index

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-21 14:18 [PATCH 0/3] add actions to modify " Dekel Peled
2019-03-21 14:18 ` [PATCH 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-03-26  9:24   ` Dekel Peled
2019-03-29 13:58   ` Adrien Mazarguil
2019-03-31 13:09     ` Dekel Peled
2019-03-21 14:18 ` [PATCH 2/3] app/testpmd: " Dekel Peled
2019-03-29 13:58   ` Adrien Mazarguil
2019-03-31 13:10     ` Dekel Peled
2019-03-21 14:18 ` [PATCH 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-04-02 15:13 ` [PATCH v2 0/3] add actions to modify header fields Dekel Peled
2019-04-10 11:26   ` [dpdk-dev] [PATCH v3 " Dekel Peled
2019-04-10 11:26     ` [dpdk-dev] [PATCH v3 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-04-10 11:26     ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: " Dekel Peled
2019-04-10 11:26     ` [dpdk-dev] [PATCH v3 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-04-10 11:50     ` [dpdk-dev] [PATCH v4 0/3] add actions to modify header fields Dekel Peled
2019-04-10 11:50       ` [dpdk-dev] [PATCH v4 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-04-18 12:30         ` Adrien Mazarguil
2019-04-22  7:15           ` Dekel Peled
2019-04-10 11:50       ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: " Dekel Peled
2019-04-10 11:50       ` [dpdk-dev] [PATCH v4 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-04-22 11:22       ` [dpdk-dev] [PATCH v5 0/3] add actions to modify header fields Dekel Peled
2019-04-22 11:22         ` [dpdk-dev] [PATCH v5 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-04-22 11:22         ` [dpdk-dev] [PATCH v5 2/3] app/testpmd: " Dekel Peled
2019-04-22 11:22         ` [dpdk-dev] [PATCH v5 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-06-02  8:18         ` [dpdk-dev] [PATCH v5 0/3] add actions to modify header fields Dekel Peled
2019-06-04  5:13           ` Dekel Peled
2019-06-04  8:14             ` Dekel Peled
2019-06-17  6:12         ` [dpdk-dev] [PATCH v6 " Dekel Peled
2019-06-17  6:12           ` [dpdk-dev] [PATCH v6 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-06-17  6:12           ` [dpdk-dev] [PATCH v6 2/3] app/testpmd: " Dekel Peled
2019-06-17  6:12           ` [dpdk-dev] [PATCH v6 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-06-27 17:39           ` [dpdk-dev] [PATCH v7 0/3] add actions to modify header fields Dekel Peled
2019-06-30  7:59             ` [dpdk-dev] [PATCH v8 " Dekel Peled
2019-06-30  7:59               ` [dpdk-dev] [PATCH v8 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-07-01  8:55                 ` Adrien Mazarguil
2019-07-01  9:58                   ` Dekel Peled
2019-06-30  7:59               ` [dpdk-dev] [PATCH v8 2/3] app/testpmd: " Dekel Peled
2019-06-30  7:59               ` [dpdk-dev] [PATCH v8 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
     [not found]           ` <cover.1561656977.git.dekelp@mellanox.com>
2019-06-27 17:39             ` [dpdk-dev] [PATCH v7 1/3] ethdev: add actions to modify TCP header fields Dekel Peled
2019-06-27 17:54               ` Andrew Rybchenko
2019-06-28 16:18                 ` Adrien Mazarguil
2019-06-27 17:39             ` [dpdk-dev] [PATCH v7 2/3] app/testpmd: " Dekel Peled
2019-06-27 17:39             ` [dpdk-dev] [PATCH v7 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-04-02 15:13 ` [PATCH v2 1/3] ethdev: add actions to modify TCP header fields Dekel Peled
2019-04-02 16:33   ` Ori Kam
2019-04-03  9:14   ` Adrien Mazarguil
2019-04-03 10:49     ` Dekel Peled
2019-04-03 12:49       ` Adrien Mazarguil
2019-04-04  9:01         ` Ori Kam
2019-04-04 13:25           ` Adrien Mazarguil
2019-04-05 11:54             ` [dpdk-dev] " Andrew Rybchenko
2019-04-08 13:36             ` Dekel Peled [this message]
2019-04-08 13:53               ` Andrew Rybchenko
2019-04-08 14:21                 ` Adrien Mazarguil
2019-04-02 15:13 ` [PATCH v2 2/3] app/testpmd: " Dekel Peled
2019-04-02 16:33   ` Ori Kam
2019-04-02 15:13 ` [PATCH v2 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-04-02 16:34   ` Ori Kam
2019-04-03  8:27   ` Shahaf Shuler
2019-07-01 15:43 ` [dpdk-dev] [PATCH v9 0/3] add actions to modify header fields Dekel Peled
2019-07-01 15:43   ` [dpdk-dev] [PATCH v9 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-07-02  8:14     ` Andrew Rybchenko
2019-07-02  9:52       ` Dekel Peled
2019-07-02 10:33         ` Adrien Mazarguil
2019-07-02 12:01           ` Dekel Peled
2019-07-01 15:43   ` [dpdk-dev] [PATCH v9 2/3] app/testpmd: " Dekel Peled
2019-07-01 15:43   ` [dpdk-dev] [PATCH v9 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-07-02 14:44 ` [dpdk-dev] [PATCH v10 0/3] add actions to modify header fields Dekel Peled
2019-07-02 14:44   ` [dpdk-dev] [PATCH v10 1/3] ethdev: add actions to modify TCP " Dekel Peled
2019-07-03  5:04     ` Slava Ovsiienko
2019-07-02 14:44   ` [dpdk-dev] [PATCH v10 2/3] app/testpmd: " Dekel Peled
2019-07-03  6:30     ` Slava Ovsiienko
2019-07-02 14:44   ` [dpdk-dev] [PATCH v10 3/3] net/mlx5: update modify header using Direct Verbs Dekel Peled
2019-07-03  6:30     ` Slava Ovsiienko
2019-07-02 15:15   ` [dpdk-dev] [PATCH v10 0/3] add actions to modify header fields Adrien Mazarguil
2019-07-03 14:59     ` Ferruh Yigit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR05MB4224AB74A0DB51996BEFD8EFB62C0@VI1PR05MB4224.eurprd05.prod.outlook.com \
    --to=dekelp@mellanox.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=arybchenko@solarflare.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=jingjing.wu@intel.com \
    --cc=orika@mellanox.com \
    --cc=shahafs@mellanox.com \
    --cc=wenzhuo.lu@intel.com \
    --cc=yskoh@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git