[v4,1/3] ethdev: add actions to modify TCP header fields
Checks
Commit Message
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 | 4 +++
lib/librte_ethdev/rte_flow.h | 60 +++++++++++++++++++++++++++++++
3 files changed, 136 insertions(+)
Comments
On Wed, Apr 10, 2019 at 02:50:41PM +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>
Almost there... Some changes were not made as discussed, please see below.
<snip>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 0203f4f..fc234de 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.
> +
> +Using this action on non-matching traffic will result in undefined behavior,
> +depending on PMD implementation.
OK, "depending on PMD implementation" looks a bit redundant though.
> +
> +.. _table_rte_flow_action_inc_tcp_seq:
> +
> +.. table:: INC_TCP_SEQ
> +
> + +-----------+------------------------------------------+
> + | Field | Value |
> + +===========+==========================================+
> + | ``value`` | Value to increase TCP sequence number by |
> + +-----------+------------------------------------------+
Configuration object documentation needs updating since you changed its type
and fields.
These comments also apply to DEC_TCP_SEQ, INC_TCP_ACK and DEC_TCP_ACK.
<snip>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index c0fe879..e3f6210 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1651,6 +1651,46 @@ 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.
> + *
> + * Using this action on non-matching traffic will result in
> + * undefined behavior, depending on PMD implementation.
"depending on PMD implementation" also redundant here.
<snip>
> /*
> + * @warning
> + * @b EXPERIMENTAL: this union may change without prior notice
> + *
> + * General integer type, can be expanded as needed.
> + */
> +union rte_flow_integer {
> + rte_be32_t be32;
> +};
You must include the extra fields you don't use (64/16/32/8 and
le/be/host/signed variants as described in previous messages) to limit the
risk of ABI breakage next time someone needs a field that isn't there yet.
This object must be able to accommodate the largest supported integer and
have the proper alignment constraints for any of these types, hence the
union with all of them from the start.
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * General struct, for use by actions that require a single integer value.
> + */
> +struct rte_flow_general_action {
> + union rte_flow_integer integer;
> +};
Seriously, why can't actions take "union rte_flow_integer" directly? Besides
"rte_flow_general_action" is vague as heck.
Seems like you made this structure so it could be extended later, but forgot
that doing so is not an option since ABIs are set in stone. You must make
new APIs as exhaustive and restrict their scope as much as possible from the
start because of that.
Note if you don't want to use union rte_flow_integer directly, it's also
fine to document your actions as taking (const rte_be32_t *) directly
through their conf pointer.
Thanks, PSB.
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, April 18, 2019 3:31 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>; arybchenko@solarflare.com;
> dev@dpdk.org; Ori Kam <orika@mellanox.com>
> Subject: Re: [PATCH v4 1/3] ethdev: add actions to modify TCP header fields
>
> On Wed, Apr 10, 2019 at 02:50:41PM +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>
>
> Almost there... Some changes were not made as discussed, please see
> below.
>
> <snip>
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 0203f4f..fc234de 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.
> > +
> > +Using this action on non-matching traffic will result in undefined
> > +behavior, depending on PMD implementation.
>
> OK, "depending on PMD implementation" looks a bit redundant though.
Fine, will remove it.
>
> > +
> > +.. _table_rte_flow_action_inc_tcp_seq:
> > +
> > +.. table:: INC_TCP_SEQ
> > +
> > + +-----------+------------------------------------------+
> > + | Field | Value |
> > +
> +===========+==========================================+
> > + | ``value`` | Value to increase TCP sequence number by |
> > + +-----------+------------------------------------------+
>
> Configuration object documentation needs updating since you changed its
> type and fields.
>
> These comments also apply to DEC_TCP_SEQ, INC_TCP_ACK and
> DEC_TCP_ACK.
Will update.
>
> <snip>
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index c0fe879..e3f6210 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1651,6 +1651,46 @@ 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.
> > + *
> > + * Using this action on non-matching traffic will result in
> > + * undefined behavior, depending on PMD implementation.
>
> "depending on PMD implementation" also redundant here.
>
> <snip>
> > /*
> > + * @warning
> > + * @b EXPERIMENTAL: this union may change without prior notice
> > + *
> > + * General integer type, can be expanded as needed.
> > + */
> > +union rte_flow_integer {
> > + rte_be32_t be32;
> > +};
>
> You must include the extra fields you don't use (64/16/32/8 and
> le/be/host/signed variants as described in previous messages) to limit the
> risk of ABI breakage next time someone needs a field that isn't there yet.
>
> This object must be able to accommodate the largest supported integer and
> have the proper alignment constraints for any of these types, hence the
> union with all of them from the start.
I will add required fields for 8, 16 and 32 bits.
Adding 64 bit fields will inflate the union size, wasting memory.
It should be handled separately in specific cases.
>
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * General struct, for use by actions that require a single integer value.
> > + */
> > +struct rte_flow_general_action {
> > + union rte_flow_integer integer;
> > +};
>
> Seriously, why can't actions take "union rte_flow_integer" directly? Besides
> "rte_flow_general_action" is vague as heck.
>
I prefer to follow the existing convention, where actions take struct even if it contains a single field.
I will define the union unnamed in the struct.
I will rename to rte_flow_integer_action.
> Seems like you made this structure so it could be extended later, but forgot
> that doing so is not an option since ABIs are set in stone. You must make new
> APIs as exhaustive and restrict their scope as much as possible from the start
> because of that.
>
> Note if you don't want to use union rte_flow_integer directly, it's also fine to
> document your actions as taking (const rte_be32_t *) directly through their
> conf pointer.
>
> --
> Adrien Mazarguil
> 6WIND
@@ -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.
+
+Using this action on non-matching traffic will result in undefined behavior,
+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.
+
+Using this action on non-matching traffic will result in undefined behavior,
+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.
+
+Using this action on non-matching traffic will result in undefined behavior,
+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.
+
+Using this action on non-matching traffic will result in undefined behavior,
+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
~~~~~~~~~~~~~~
@@ -143,6 +143,10 @@ 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_general_action)),
+ MK_FLOW_ACTION(DEC_TCP_SEQ, sizeof(struct rte_flow_general_action)),
+ MK_FLOW_ACTION(INC_TCP_ACK, sizeof(struct rte_flow_general_action)),
+ MK_FLOW_ACTION(DEC_TCP_ACK, sizeof(struct rte_flow_general_action)),
};
static int
@@ -1651,6 +1651,46 @@ 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.
+ *
+ * Using this action on non-matching traffic will result in
+ * undefined behavior, depending on PMD implementation.
+ *
+ * See struct rte_flow_general_action.
+ */
+ RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ,
+
+ /**
+ * Decrease sequence number in the outermost TCP header.
+ *
+ * Using this action on non-matching traffic will result in
+ * undefined behavior, depending on PMD implementation.
+ *
+ * See struct rte_flow_general_action.
+ */
+ RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ,
+
+ /**
+ * Increase acknowledgment number in the outermost TCP header.
+ *
+ * Using this action on non-matching traffic will result in
+ * undefined behavior, depending on PMD implementation.
+ *
+ * See struct rte_flow_general_action.
+ */
+ RTE_FLOW_ACTION_TYPE_INC_TCP_ACK,
+
+ /**
+ * Decrease acknowledgment number in the outermost TCP header.
+ *
+ * Using this action on non-matching traffic will result in
+ * undefined behavior, depending on PMD implementation.
+ *
+ * See struct rte_flow_general_action.
+ */
+ RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK,
};
/**
@@ -2123,6 +2163,26 @@ struct rte_flow_action_set_mac {
};
/*
+ * @warning
+ * @b EXPERIMENTAL: this union may change without prior notice
+ *
+ * General integer type, can be expanded as needed.
+ */
+union rte_flow_integer {
+ rte_be32_t be32;
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
+ * General struct, for use by actions that require a single integer value.
+ */
+struct rte_flow_general_action {
+ union rte_flow_integer integer;
+};
+
+/*
* Definition of a single action.
*
* A list of actions is terminated by a END action.