DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Ori Kam <orika@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
	Dekel Peled <dekelp@mellanox.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: [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> (raw)
In-Reply-To: <20190403124921.GR4889@6wind.com>

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 fields
> 
> 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 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>
> > > <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 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.

> > > <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 structure 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.
> 
> 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.
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.

So I vote to keep Dekel patch as is.

> --
> Adrien Mazarguil
> 6WIND


Ori Kam

  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 [this message]
2019-04-04 13:25           ` Adrien Mazarguil
2019-04-05 11:54             ` [dpdk-dev] " Andrew Rybchenko
2019-04-08 13:36             ` Dekel Peled
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=AM4PR05MB342559B6EA39BF745EC3690DDB500@AM4PR05MB3425.eurprd05.prod.outlook.com \
    --to=orika@mellanox.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dekelp@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=jingjing.wu@intel.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 dpdk-dev@archiver.kernel.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