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

[Dekel Peled <dekelp@mellanox.com>]

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