RE: [PATCH 3/3] ethdev: add structure for indirect AGE update

[Michael Baum <michaelba@nvidia.com>]

Hi Andrew and Ori,

On 10/4/22 9:37, Andrew Rybchenko wrote: 
> 
> On 10/3/22 18:13, Ori Kam wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, 3 October 2022 16:18
> >>
> >> On 10/3/22 11:30, Ori Kam wrote:
> >>> Hi Andrew,
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Monday, 3 October 2022 11:04
> >>>>
> >>>> On 9/21/22 17:54, Michael Baum wrote:
> >>>>> Add a new structure for indirect AGE update.
> >>>>>
> >>>>> This new structure enables:
> >>>>> 1. Update timeout value.
> >>>>> 2. Stop AGE checking.
> >>>>> 3. Start AGE checking.
> >>>>> 4. restart AGE checking.
> >>>>>
> >>>>> Signed-off-by: Michael Baum <michaelba@nvidia.com>
> >>>>> ---
> >>>>>     app/test-pmd/cmdline_flow.c        | 66
> >>>> ++++++++++++++++++++++++++++++
> >>>>>     app/test-pmd/config.c              | 18 +++++++-
> >>>>>     doc/guides/prog_guide/rte_flow.rst | 25 +++++++++--
> >>>>>     lib/ethdev/rte_flow.h              | 27 ++++++++++++
> >>>>>     4 files changed, 132 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
> >> pmd/cmdline_flow.c
> >>>>> index 4fb90a92cb..a315fd9ded 100644
> >>>>> --- a/app/test-pmd/cmdline_flow.c
> >>>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>>> @@ -583,6 +583,9 @@ enum index {
> >>>>>           ACTION_SET_IPV6_DSCP_VALUE,
> >>>>>           ACTION_AGE,
> >>>>>           ACTION_AGE_TIMEOUT,
> >>>>> + ACTION_AGE_UPDATE,
> >>>>> + ACTION_AGE_UPDATE_TIMEOUT,
> >>>>> + ACTION_AGE_UPDATE_TOUCH,
> >>>>>           ACTION_SAMPLE,
> >>>>>           ACTION_SAMPLE_RATIO,
> >>>>>           ACTION_SAMPLE_INDEX,
> >>>>> @@ -1869,6 +1872,7 @@ static const enum index next_action[] = {
> >>>>>           ACTION_SET_IPV4_DSCP,
> >>>>>           ACTION_SET_IPV6_DSCP,
> >>>>>           ACTION_AGE,
> >>>>> + ACTION_AGE_UPDATE,
> >>>>>           ACTION_SAMPLE,
> >>>>>           ACTION_INDIRECT,
> >>>>>           ACTION_MODIFY_FIELD,
> >>>>> @@ -2113,6 +2117,14 @@ static const enum index action_age[] = {
> >>>>>           ZERO,
> >>>>>     };
> >>>>>
> >>>>> +static const enum index action_age_update[] = {
> >>>>> +ACTION_AGE_UPDATE,  ACTION_AGE_UPDATE_TIMEOUT,
> >>>>> +ACTION_AGE_UPDATE_TOUCH,  ACTION_NEXT,  ZERO, };
> >>>>> +
> >>>>>     static const enum index action_sample[] = {
> >>>>>           ACTION_SAMPLE,
> >>>>>           ACTION_SAMPLE_RATIO,
> >>>>> @@ -2191,6 +2203,9 @@ static int parse_vc_spec(struct context *,
> >>>>> const
> >>>> struct token *,
> >>>>>                            const char *, unsigned int, void *, unsigned int);
> >>>>>     static int parse_vc_conf(struct context *, const struct token *,
> >>>>>                            const char *, unsigned int, void *,
> >>>>> unsigned int);
> >>>>> +static int parse_vc_conf_timeout(struct context *, const struct token
> *,
> >>>>> +                          const char *, unsigned int, void *,
> >>>>> +                          unsigned int);
> >>>>>     static int parse_vc_item_ecpri_type(struct context *, const
> >>>>> struct token
> >> *,
> >>>>>                                       const char *, unsigned int,
> >>>>>                                       void *, unsigned int); @@
> >>>>> -6194,6 +6209,30 @@ static const struct token token_list[] = {
> >>>>>                   .next = NEXT(action_age,
> >>>> NEXT_ENTRY(COMMON_UNSIGNED)),
> >>>>>                   .call = parse_vc_conf,
> >>>>>           },
> >>>>> + [ACTION_AGE_UPDATE] = {
> >>>>> +         .name = "age_update",
> >>>>> +         .help = "update aging parameter",
> >>>>> +         .next = NEXT(action_age_update),
> >>>>> +         .priv = PRIV_ACTION(AGE,
> >>>>> +                             sizeof(struct rte_flow_update_age)),
> >>>>> +         .call = parse_vc,
> >>>>> + },
> >>>>> + [ACTION_AGE_UPDATE_TIMEOUT] = {
> >>>>> +         .name = "timeout",
> >>>>> +         .help = "age timeout update value",
> >>>>> +         .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_update_age,
> >>>>> +                                    timeout, 24)),
> >>>>> +         .next = NEXT(action_age_update,
> >>>> NEXT_ENTRY(COMMON_UNSIGNED)),
> >>>>> +         .call = parse_vc_conf_timeout, },
> >>>>> + [ACTION_AGE_UPDATE_TOUCH] = {
> >>>>> +         .name = "touch",
> >>>>> +         .help = "this flow is touched",
> >>>>> +         .next = NEXT(action_age_update,
> >>>> NEXT_ENTRY(COMMON_BOOLEAN)),
> >>>>> +         .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_update_age,
> >>>>> +                                    touch, 1)),
> >>>>> +         .call = parse_vc_conf,
> >>>>> + },
> >>>>>           [ACTION_SAMPLE] = {
> >>>>>                   .name = "sample",
> >>>>>                   .help = "set a sample action", @@ -7031,6
> >>>>> +7070,33 @@ parse_vc_conf(struct context *ctx, const
> >> struct
> >>>> token *token,
> >>>>>           return len;
> >>>>>     }
> >>>>>
> >>>>> +/** Parse action configuration field. */ static int
> >>>>> +parse_vc_conf_timeout(struct context *ctx, const struct token
> *token,
> >>>>> +               const char *str, unsigned int len,
> >>>>> +               void *buf, unsigned int size) {  struct buffer
> >>>>> +*out = buf;  struct rte_flow_update_age *update;
> >>>>> +
> >>>>> + (void)size;
> >>>>> + if (ctx->curr != ACTION_AGE_UPDATE_TIMEOUT)
> >>>>> +         return -1;
> >>>>> + /* Token name must match. */
> >>>>> + if (parse_default(ctx, token, str, len, NULL, 0) < 0)
> >>>>> +         return -1;
> >>>>> + /* Nothing else to do if there is no buffer. */ if (!out)
> >>>>> +         return len;
> >>>>> + /* Point to selected object. */
> >>>>> + ctx->object = out->args.vc.data; objmask = NULL;
> >>>>> + /* Update the timeout is valid. */ update = (struct
> >>>>> + rte_flow_update_age *)out->args.vc.data;
> >>>>> + update->timeout_valid = 1;
> >>>>> + return len;
> >>>>> +}
> >>>>> +
> >>>>>     /** Parse eCPRI common header type field. */
> >>>>>     static int
> >>>>>     parse_vc_item_ecpri_type(struct context *ctx, const struct
> >>>>> token
> >> *token,
> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>>> 31952467fb..45495385d7 100644
> >>>>> --- a/app/test-pmd/config.c
> >>>>> +++ b/app/test-pmd/config.c
> >>>>> @@ -2065,6 +2065,7 @@ port_action_handle_update(portid_t
> port_id,
> >>>> uint32_t id,
> >>>>>           if (!pia)
> >>>>>                   return -EINVAL;
> >>>>>           switch (pia->type) {
> >>>>> + case RTE_FLOW_ACTION_TYPE_AGE:
> >>>>>           case RTE_FLOW_ACTION_TYPE_CONNTRACK:
> >>>>>                   update = action->conf;
> >>>>>                   break;
> >>>>> @@ -2904,6 +2905,8 @@
> port_queue_action_handle_update(portid_t
> >>>> port_id,
> >>>>>           struct rte_port *port;
> >>>>>           struct rte_flow_error error;
> >>>>>           struct rte_flow_action_handle *action_handle;
> >>>>> + struct port_indirect_action *pia; const void *update;
> >>>>>
> >>>>>           action_handle = port_action_handle_get_by_id(port_id, id);
> >>>>>           if (!action_handle)
> >>>>> @@ -2915,8 +2918,21 @@
> port_queue_action_handle_update(portid_t
> >>>> port_id,
> >>>>>                   return -EINVAL;
> >>>>>           }
> >>>>>
> >>>>> + pia = action_get_by_id(port_id, id); if (!pia)
> >>>>> +         return -EINVAL;
> >>>>> +
> >>>>> + switch (pia->type) {
> >>>>> + case RTE_FLOW_ACTION_TYPE_AGE:
> >>>>> +         update = action->conf;
> >>>>> +         break;
> >>>>> + default:
> >>>>> +         update = action;
> >>>>> +         break;
> >>>>> + }
> >>>>> +
> >>>>>           if (rte_flow_async_action_handle_update(port_id, queue_id,
> &attr,
> >>>>> -                             action_handle, action, NULL, &error)) {
> >>>>> +                             action_handle, update, NULL,
> >>>>> + &error)) {
> >>>>>                   return port_flow_complain(&error);
> >>>>>           }
> >>>>>           printf("Indirect action #%u update queued\n", id); diff
> >>>>> --git a/doc/guides/prog_guide/rte_flow.rst
> >>>> b/doc/guides/prog_guide/rte_flow.rst
> >>>>> index 588914b231..dae9121279 100644
> >>>>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>>>> @@ -2958,7 +2958,7 @@ Otherwise,
> RTE_FLOW_ERROR_TYPE_ACTION
> >>>> error will be returned.
> >>>>>     Action: ``AGE``
> >>>>>     ^^^^^^^^^^^^^^^
> >>>>>
> >>>>> -Set ageing timeout configuration to a flow.
> >>>>> +Set aging timeout configuration to a flow.
> >>>>>
> >>>>>     Event RTE_ETH_EVENT_FLOW_AGED will be reported if
> >>>>>     timeout passed without any matching on the flow.
> >>>>> @@ -2977,8 +2977,8 @@ timeout passed without any matching on
> the
> >>>> flow.
> >>>>>        | ``context``  | user input flow context         |
> >>>>>        +--------------+---------------------------------+
> >>>>>
> >>>>> -Query structure to retrieve ageing status information of a
> >>>>> -shared AGE action, or a flow rule using the AGE action:
> >>>>> +Query structure to retrieve aging status information of an
> >>>>> +indirect AGE action, or a flow rule using the AGE action:
> >>>>>
> >>>>>     .. _table_rte_flow_query_age:
> >>>>>
> >>>>> @@ -2994,6 +2994,25 @@ shared AGE action, or a flow rule using the
> >> AGE
> >>>> action:
> >>>>>        | ``sec_since_last_hit``       | out | Seconds since last traffic hit         |
> >>>>>
> >>>>> +------------------------------+-----+----------------------------
> >>>>> ------------+
> >>>>>
> >>>>> +Update structure to modify the parameters of an indirect AGE
> action.
> >>>>> +The update structure is used by
> >>>>> +``rte_flow_action_handle_update()``
> >>>> function.
> >>>>> +
> >>>>> +.. _table_rte_flow_update_age:
> >>>>> +
> >>>>> +.. table:: AGE update
> >>>>> +
> >>>>> +   +-------------------+-----------------------------------------------------------
> ---+
> >>>>> +   | Field             | Value                                                        |
> >>>>> +
> >>>>
> +===================+=====================================
> >>>> =========================+
> >>>>> +   | ``timeout``       | 24 bits timeout value                                        |
> >>>>> +   +-------------------+-----------------------------------------------------------
> ---+
> >>>>> +   | ``timeout_valid`` | 1 bit, timeout value is valid                                |
> >>>>> +   +-------------------+-----------------------------------------------------------
> ---+
> >>>>> +   | ``touch``         | 1 bit, touch the AGE action to set
> >> ``sec_since_last_hit`` 0
> >>>> |
> >>>>> +   +-------------------+-----------------------------------------------------------
> ---+
> >>>>> +   | ``reserved``      | 6 bits reserved, must be zero                                |
> >>>>> +
> >>>>> + +-------------------+-------------------------------------------
> >>>>> + -------------------+
> >>>>> +
> >>>>>     Action: ``SAMPLE``
> >>>>>     ^^^^^^^^^^^^^^^^^^
> >>>>>
> >>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> >>>>> d830b02321..a21d437cf8 100644
> >>>>> --- a/lib/ethdev/rte_flow.h
> >>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>> @@ -2954,6 +2954,33 @@ struct rte_flow_query_age {
> >>>>>           uint32_t sec_since_last_hit:24; /**< Seconds since last traffic hit.
> */
> >>>>>     };
> >>>>>
> >>>>> +/**
> >>>>> + * @warning
> >>>>> + * @b EXPERIMENTAL: this structure may change without prior
> >>>>> +notice
> >>>>> + *
> >>>>> + * RTE_FLOW_ACTION_TYPE_AGE
> >>>>> + *
> >>>>> + * Update indirect AGE action attributes:
> >>>>> + *  - Timeout can be updated including stop/start action:
> >>>>> + *     +-------------+-------------+------------------------------+
> >>>>> + *     | Old Timeout | New Timeout | Updating                     |
> >>>>> + *
> >>>>
> +=============+=============+=============================
> >>>> =+
> >>>>> + *     | 0           | positive    | Start aging with new value   |
> >>>>> + *     +-------------+-------------+------------------------------+
> >>>>> + *     | positive    | 0           | Stop aging                    |
> >>>>> + *     +-------------+-------------+------------------------------+
> >>>>> + *     | positive    | positive    | Change timeout to new value  |
> >>>>> + *     +-------------+-------------+------------------------------+
> >>>>> + *  - sec_since_last_hit can be reset.
> >>>>> + */
> >>>>> +struct rte_flow_update_age {
> >>>>
> >>>> I think it worse to mention the structure in
> >>>> RTE_FLOW_ACTION_TYPE_AGE description.
> >>>
> >>> What do you mean?
> >>
> >> RTE_FLOW_ACTION_TYPE_AGE has a description as enum member.
> >> Typically configuration structures are mentioned there.
> >> However, I think that it would be useful to mention specific update
> >> structure if any. Like this one. Just to make it clear that if a user
> >> wants to update the action configuration it should use specific
> >> structure. Or am I missing something?
> >> Misunderstand something?
> >>
> >
> > Do you mean something like:
> > * see enum RTE_ETH_EVENT_FLOW_AGED
> >   * See struct rte_flow_query_age
> > + See struct rte_flow_update_age
> >   */
> > RTE_FLOW_ACTION_TYPE_AGE,
> >   If so, I fully agree.
> 
> Yes, exactly.

I have updated it according your suggestion, thank you.

> 
> >
> >>>
> >>>>
> >>>>> + uint32_t reserved:6; /**< Reserved, must be zero. */ uint32_t
> >>>>> + timeout_valid:1; /**< The timeout is valid for update. */
> >>>>> + uint32_t timeout:24; /**< Time in seconds. */ uint32_t touch:1;
> >>>>> + /**< Means that aging should assume packet passed the aging. */
> >>>>
> >>>> Documentation after code makes sense if it is in the same line.
> >>>> If the documentation is in its own line, it should be before the
> >>>> code and use /** prefix.

Updated, thanks.

> >>>
> >>> +1
> >>>
> >>>> One more question: why order in the documentation above does not
> >>>> match order here?

I updated the documentation to be align to this order.

> >>>
> >>> I think we can remove it from the documentation since it doesn't add
> >> anything
> >>>
> >>>>> +};
> >>>>> +
> >>>>>     /**
> >>>>>      * @warning
> >>>>>      * @b EXPERIMENTAL: this structure may change without prior
> >>>>> notice
> >>>
> >