Re: [dpdk-dev] [PATCH v4 1/3] ethdev: add actions to modify TCP header fields

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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.

-- 
Adrien Mazarguil
6WIND