[v2,1/3] ethdev: add actions to modify TCP header fields

  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>
---
 doc/guides/prog_guide/rte_flow.rst | 72 ++++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_flow.c       |  8 +++++
 lib/librte_ethdev/rte_flow.h       | 70 ++++++++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> Sent: Tuesday, April 2, 2019 6:13 PM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; wenzhuo.lu@intel.com;
> jingjing.wu@intel.com; bernard.iremonger@intel.com; Yongseok Koh
> <yskoh@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Dekel Peled
> <dekelp@mellanox.com>
> Subject: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header
> fields
> 
> 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>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 72
> ++++++++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_flow.c       |  8 +++++
>  lib/librte_ethdev/rte_flow.h       | 70
> ++++++++++++++++++++++++++++++++++++
>  3 files changed, 150 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 0203f4f..5bebb29 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2345,6 +2345,78 @@ Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error
> will be returned.
>     | ``mac_addr`` | MAC address   |
>     +--------------+---------------+
> 
> +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.
> +
> +.. _table_rte_flow_action_inc_tcp_seq:
> +
> +.. table:: INC_TCP_SEQ
> +
> +   +-----------+------------------------------------------+
> +   | Field     | Value                                    |
> +   +===========+==========================================+
> +   | ``value`` | Value to increase TCP sequence number by |
> +   +-----------+------------------------------------------+
> +
> +Action: ``DEC_TCP_SEQ``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Decrease 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.
> +
> +.. _table_rte_flow_action_dec_tcp_seq:
> +
> +.. table:: DEC_TCP_SEQ
> +
> +   +-----------+------------------------------------------+
> +   | Field     | Value                                    |
> +   +===========+==========================================+
> +   | ``value`` | Value to decrease TCP sequence number by |
> +   +-----------+------------------------------------------+
> +
> +Action: ``INC_TCP_ACK``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Increase acknowledgment 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.
> +
> +.. _table_rte_flow_action_inc_tcp_ack:
> +
> +.. table:: INC_TCP_ACK
> +
> +   +-----------+------------------------------------------------+
> +   | Field     | Value                                          |
> +   +===========+================================================+
> +   | ``value`` | Value to increase TCP acknowledgment number by |
> +   +-----------+------------------------------------------------+
> +
> +Action: ``DEC_TCP_ACK``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Decrease acknowledgment 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.
> +
> +.. _table_rte_flow_action_dec_tcp_ack:
> +
> +.. table:: DEC_TCP_ACK
> +
> +   +-----------+------------------------------------------------+
> +   | Field     | Value                                          |
> +   +===========+================================================+
> +   | ``value`` | Value to decrease TCP acknowledgment number by |
> +   +-----------+------------------------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 3277be1..589d0b9 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -143,6 +143,14 @@ struct rte_flow_desc_data {
>  	MK_FLOW_ACTION(SET_TTL, sizeof(struct rte_flow_action_set_ttl)),
>  	MK_FLOW_ACTION(SET_MAC_SRC, sizeof(struct
> rte_flow_action_set_mac)),
>  	MK_FLOW_ACTION(SET_MAC_DST, sizeof(struct
> rte_flow_action_set_mac)),
> +	MK_FLOW_ACTION(INC_TCP_SEQ,
> +			sizeof(struct rte_flow_action_modify_tcp_seq)),
> +	MK_FLOW_ACTION(DEC_TCP_SEQ,
> +			sizeof(struct rte_flow_action_modify_tcp_seq)),
> +	MK_FLOW_ACTION(INC_TCP_ACK,
> +			sizeof(struct rte_flow_action_modify_tcp_ack)),
> +	MK_FLOW_ACTION(DEC_TCP_ACK,
> +			sizeof(struct rte_flow_action_modify_tcp_ack)),
>  };
> 
>  static int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index c0fe879..eca7544 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1651,6 +1651,50 @@ enum rte_flow_action_type {
>  	 * See struct rte_flow_action_set_mac.
>  	 */
>  	RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> +
> +	/**
> +	 * 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.
> +	 *
> +	 * See struct rte_flow_action_modify_tcp_seq
> +	 */
> +	RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ,
> +
> +	/**
> +	 * Decrease 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.
> +	 *
> +	 * See struct rte_flow_action_modify_tcp_seq
> +	 */
> +	RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ,
> +
> +	/**
> +	 * Increase acknowledgment 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.
> +	 *
> +	 * See struct rte_flow_action_modify_tcp_ack
> +	 */
> +	RTE_FLOW_ACTION_TYPE_INC_TCP_ACK,
> +
> +	/**
> +	 * Decrease acknowledgment 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.
> +	 *
> +	 * See struct rte_flow_action_modify_tcp_ack
> +	 */
> +	RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK,
>  };
> 
>  /**
> @@ -2122,6 +2166,32 @@ struct rte_flow_action_set_mac {
>  	uint8_t mac_addr[ETHER_ADDR_LEN];
>  };
> 
> +/**
> + * @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. */
> +};
> +
>  /*
>   * Definition of a single action.
>   *
> --
> 1.8.3.1


Thanks,
Acked-by: Ori Kam <orika@mellanox.com>
  

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.

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

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.

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

> 
> --
> Adrien Mazarguil
> 6WIND
  

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?
  

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
  

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

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

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 :)
  

On 4/4/19 4:25 PM, Adrien Mazarguil wrote:
> 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
> 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.
>
> <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?

I like the idea (plus 16-bit options). We already have too many
rte_flow_action_* structures with single integer in it.

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

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)? IMHO, having INC and DEC is too artificial and just bloat API
and code.

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

Too many actions bloat the code everywhere: API, PMDs, testpmd, other apps.
If it is possible to decrease number of different actions without
over-complicating, it should be done.

>> So I vote to keep Dekel patch as is.
> I don't, I guess another vote is needed to decide :)
>
  

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
  

On 4/8/19 4:36 PM, Dekel Peled wrote:
> 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.

I think it is an overkill to have two actions for the purpose: DEC 
(value) == INC ((uint32_t)-value)
If it is really important to have DEC and INC, please, make it clear 
from actions documentation why.

Thanks,
Andrew.
  

Hi Andrew, *Dekel* (I swear I'm not doing it on purpose, hopefully I won't
make that stupid mistake again :)

On Mon, Apr 08, 2019 at 04:53:54PM +0300, Andrew Rybchenko wrote:
> On 4/8/19 4:36 PM, Dekel Peled wrote:
> > 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.
> 
> I think it is an overkill to have two actions for the purpose: DEC (value)
> == INC ((uint32_t)-value)
> If it is really important to have DEC and INC, please, make it clear from
> actions documentation why.

The main reason in my opinion is that a signed value may not be able to
represent an increment large enough for an unsigned value of the same bit
width.

This can be worked around by using a type larger than the underlying data
field (e.g. i64 for u32), but it will look confusing and is not an option
for the largest unsigned type we support (u64).

Another problem is what endian increment/decrement actions should use. There
are no dedicated endian types for signed values at the moment, and I'm not
sure we should define any.

This could be addressed by defining a third "SET" action to overwrite the
current value (even if unused) as this action will use the same type as
the two others and that of the underlying data (including endianness) for
consistency.