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

  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(-)
  

Hi Michael,


> -----Original Message-----
> From: Michael Baum <michaelba@nvidia.com>
> Sent: Wednesday, 21 September 2022 17:54
> 
> 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>
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori
  

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;
> +	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.

> +	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.

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

> +};
> +
>   /**
>    * @warning
>    * @b EXPERIMENTAL: this structure may change without prior notice
  

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;
> > +	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?

> 
> > +	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
  

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;
>>> +	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?

> 
>>
>>> +	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
>
  

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;
> >>> +	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
> >
  

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;
>>>>> +	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.

Yes, exactly.

> 
>>>
>>>>
>>>>> +	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
>>>
>
  

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
> >>>
> >