Thanks, PSB.
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> Sent: Thursday, April 18, 2019 3:31 PM
> To: Dekel Peled <dek...@mellanox.com>
> Cc: wenzhuo...@intel.com; jingjing...@intel.com;
> bernard.iremon...@intel.com; Yongseok Koh <ys...@mellanox.com>;
> Shahaf Shuler <shah...@mellanox.com>; arybche...@solarflare.com;
> dev@dpdk.org; Ori Kam <or...@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 <dek...@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
