RE: [PATCH v2 01/11] ethdev: add flex item modify field support

[Rongwei Liu <rongweil@nvidia.com>]

Hi Andrew

BR
Rongwei

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, January 20, 2023 17:08
> To: Rongwei Liu <rongweil@nvidia.com>; Matan Azrad <matan@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Aman
> Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: Re: [PATCH v2 01/11] ethdev: add flex item modify field support
> 
> External email: Use caution opening links or attachments
> 
> 
> On 1/19/23 07:58, Rongwei Liu wrote:
> > Add flex item as modify field destination.
> > Add "struct rte_flow_item_flex_handle *flex_handle" into "struct
> > rte_flow_action_modify_data" as union with existed "level" member.
> > This new member is dedicated for modifying flex item.
> >
> > Add flex item modify field cmdline support. Now user can use testpmd
> > cli to specify which flex item to be modified, either source or
> > destination.
> >
> > Syntax is as below:
> > modify_field op set dst_type flex_item dst_level 0 dst_offset 16
> > src_type value src_value 0x123456781020 width 8
> >
> > Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> 
> [snip]
> 
> > diff --git a/doc/guides/rel_notes/release_23_03.rst
> > b/doc/guides/rel_notes/release_23_03.rst
> > index b8c5b68d6c..c673205e5e 100644
> > --- a/doc/guides/rel_notes/release_23_03.rst
> > +++ b/doc/guides/rel_notes/release_23_03.rst
> > @@ -56,6 +56,10 @@ New Features
> >        =======================================================
> >
> >
> 
> It should be just one empty line here
> 
Sure.
> > +* ethdev: added a new field:
> 
> "added a new field' is too generic.
> 
> > +
> > +  - modify flex item: ``rte_flow_action_modify_data.flex_handle``
> > +
> 
> And two empty lines here.
> 
Sure.
> >   Removed Items
> >   -------------
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> > b60987db4b..c66a65351d 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -3528,6 +3528,7 @@ enum rte_flow_field_id {
> >       RTE_FLOW_FIELD_IPV6_ECN,        /**< IPv6 ECN. */
> >       RTE_FLOW_FIELD_GTP_PSC_QFI,     /**< GTP QFI. */
> >       RTE_FLOW_FIELD_METER_COLOR,     /**< Meter color marker. */
> > +     RTE_FLOW_FIELD_FLEX_ITEM,       /**< Flex item. */
> >   };
> >
> >   /**
> > @@ -3541,8 +3542,11 @@ struct rte_flow_action_modify_data {
> >       RTE_STD_C11
> >       union {
> >               struct {
> > -                     /** Encapsulation level or tag index. */
> > -                     uint32_t level;
> > +                     /**< Encapsulation level or tag index or flex
> > + item handle. */
> 
> Have you tried to generate documentation? If it is a union documentation it
> should be /**, not /**<.
Sure. Sorry, I followed wrong existed examples.
> In general, it is better to document union from overall point of view. What is it
> logically? Do not define union as just a union of its fields.
Currently, 'flex_handle' is documents in rte_flow.rst file " table:: destination/source field definition" as a new row.  
From API aspect, when modifying flex item, user should specify the pointer of the flex item instead of ID.
That' why it was added as a union. 
> 
> > +                     union {
> > +                             uint32_t level;
> > +                             struct rte_flow_item_flex_handle
> > + *flex_handle;
> 
> Union items documentation missing.
See above. Do we need another place to document the union again?
> 
> > +                     };
> >                       /** Number of bits to skip from a field. */
> >                       uint32_t offset;
> >               };