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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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