Re: [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic modify rte flow action

[Slava Ovsiienko <viacheslavo@nvidia.com>]

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Friday, January 15, 2021 20:04
> To: Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; ferruh.yigit@intel.com;
> andrew.rybchenko@oktetlabs.ru; jerinjacobk@gmail.com
> Subject: RE: [PATCH v5 1/2] ethdev: introduce generic modify rte flow action
> 
> Hi Alexander,
> 
> Small inline, but I leave it to you if you want to change it.
> 
> > -----Original Message-----
> > From: Alexander Kozyrev <akozyrev@nvidia.com>
> > Sent: Friday, January 15, 2021 5:43 PM
> > Subject: [PATCH v5 1/2] ethdev: introduce generic modify rte flow
> > action
> >
> > Implement the generic modify flow API to allow manipulations on an
> > arbitrary header field (as well as mark, metadata or tag) using data
> > from another field or a user-specified value.
> >
> > This generic modify mechanism removes the necessity to implement a
> > separate RTE Flow action every time we need to modify a new packet
> > field in the future. A user-provided value can be used from a
> > specified packet field/tag/metadata/mark/memory location or directly
> >
>  provided by a user.
> >
> 
> [Snip]
> 
> > +
> > +enum rte_flow_modify_op {
> > +	RTE_FLOW_MODIFY_MOV = 0,
> > +	RTE_FLOW_MODIFY_ADD,
> > +	RTE_FLOW_MODIFY_SUB,
> > +};
> > +
> I think  mov is not the best option, since we the value in src stays I think copy
> or set will be better.
> If you change it feel free to add my ack.

MOV is quite common usage (in Assembly) to specify  exactly the operation
we are going to introduce.

And we are going to assign the destination both from field and some immediate
value provided. So, MOV in my opinion would be more relevant than COPY.

With best regards,
Slava

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
> > + *
> > + * Modifies a destination header field according to the specified
> > + * operation. Another packet field can be used as a source as well
> > + * as tag, mark, metadata or an immediate value or a pointer to it.
> > + * Width is the number of bits used from the source item.
> > + */
> > +struct rte_flow_action_modify_field {
> > +	enum rte_flow_modify_op operation;
> > +	struct rte_flow_action_modify_data dst;
> > +	struct rte_flow_action_modify_data src;
> > +	uint32_t width;
> > +};
> > +
> >  /* Mbuf dynamic field offset for metadata. */  extern int32_t
> > rte_flow_dynf_metadata_offs;
> >
> > --
> > 2.24.1
> 
> Acked-by: Ori Kam <orika@nvidia.com>
> Best,
> Ori