Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@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.rybche...@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 <michae...@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;
> >>> + ctx->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.
> >
> >>
> >>> + 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.
> >>
> >
> > +1
> >
> >> One more question: why order in the documentation above does
> >> not match order here?
> >>
> >
> > 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
> >
