DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: 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>, Ori Kam <orika@mellanox.com>
Subject: Re: [PATCH v2 1/3] ethdev: add actions to modify TCP header fields
Date: Wed, 3 Apr 2019 14:49:21 +0200
Message-ID: <20190403124921.GR4889@6wind.com> (raw)
In-Reply-To: <VI1PR05MB4224753382F523CD7C238A7EB6570@VI1PR05MB4224.eurprd05.prod.outlook.com>

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.

> > <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?

-- 
Adrien Mazarguil
6WIND

  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 [this message]
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
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=20190403124921.GR4889@6wind.com \
    --to=adrien.mazarguil@6wind.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dekelp@mellanox.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 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