Re: [PATCH v5] ethdev: add template table resize API

[Thomas Monjalon <thomas@monjalon.net>]

11/02/2024 10:30, Gregory Etelson:
> --- a/doc/guides/howto/rte_flow.rst
> +++ b/doc/guides/howto/rte_flow.rst
> +Template API resizable table
> +----------------------------
> +
> +Description
> +~~~~~~~~~~~
> +
> +A guide to the resizable template table API.

This sentence is useless. Let's keep it as short as we can.

> +
> +The resizable template table API enables applications to dynamically adjust
> +capacity of template tables without disrupting the existing flows operation.
> +The resizable template table API allows applications to optimize the memory
> +usage and performance of template tables according to the traffic conditions
> +and requirements.
> +
> +A typical use case for the resizable template table API

You may add a colon at the end of this sentence.
The below you may use "#" for automatic list numbering.

> +
> +  1. Create a resizable table with the initial capacity.
> +
> +  2. Change the table flows capacity.
> +
> +  3. Update flows that were created before the table update.
> +
> +  4. Complete the table resize procedure.
> +
> +When application begins to resize the table, it enters the resizable state.

I'm not sure to understand what means "enters the resizable state".

> +When application finishes resizing the table, it returns to the normal state.
> +Only a table in the normal state can be resized.

This sentence looks redundant with the following one.

> After a table is back to
> +the normal state, application can start a new resize.
> +Application can add, change or remove flow rules regardless of table state.
> +Table performance may worsen in the resizable state. Table performance must
> +recover after the table is back to the normal state.

This last sentence looks redundant.

> +
> +Table resize procedure must not interfere with flows that existed before
> +the table size changed.

Who the "must" is for? I suppose you are talking about driver requirement?

> +Flow handles must remain unchanged during table resize.

Again, it is for driver behaviour?

> +Application must be able to create new flows and modify or delete existing flows
> +regardless of the table state.

If it is by design we may say "Application is able".

> +
> +Application needs to set the `RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE` bit in

Please use double backquotes for code items.

> +the table attributes when creating a template table that can be resized.
> +The current API cannot make an existing table resizable if it was not created

In the guide, there is no old or current API.

> +with the `RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE` bit.

I think this sentence is redundant.

> +Resizable template table starts in the normal state.

It looks obvious.
Remember a guide is doc a detailed API description.

> +
> +Application can trigger the table to resize by calling
> +the `rte_flow_template_table_resize()` function. The resize process updates
> +the PMD table settings and port hardware to fit the new flows capacity.
> +The resize process must not affect the current flows functionality.
> +The resize process must not change the current flows handles.

Is it redundant with what is above?
Try to be concise.

> +Application can create new flows and modify or delete existing flows
> +while the table is resizing, but the table performance might be
> +slower than usual.

It was already explained before.
I prefer this one.

> +
> +Flows that existed before table resize are still functional after table resize.
> +However, the PMD flow resources that existed before table resize may not be
> +fully efficient after table resize.

What is "not fully efficient" exactly?

> In this case, application can combine
> +the old flow resources from before the resize with the new flow resources
> +from after the resize.
> +Application uses the `rte_flow_async_update_resized()` function call to update
> +flow resources. The flow update process does not interfere with or alter
> +the existing flow object. It only updates the PMD resources associated with that
> +flow.
> +The post-resize flow update process may conflict with application flows
> +operations, such as creation, removal or update. Therefore, performance-oriented
> +applications need to choose the best time to call for post-resize flow update.

Not perfectly clear. What must be done by the app exactly?

> +When application selects flows for the post table resize update, it can iterate
> +over all existing flows or it can keep track of the flows that need
> +to be updated.
> +Flows that were created after the `rte_flow_template_table_resize()`
> +call finished do not require an update.
> +
> +To return table to the normal state, use the
> +`rte_flow_template_table_resize_complete()`. If PMD does not require post-resize
> +flows

How do we know what the PMD requires?

> update and application does not care about PMD resources optimization,
> +application can avoid post-resize flows update and move resized table back to
> +the normal state right after the `rte_flow_template_table_resize()`.
> +Application can resize the table again when it is in the normal state.
> +
> +Testpmd commands:(wrapped for clarity)::

You should replace the first colon with a space.

> +
> +  # 1. Create resizable template table for 1 flow.
> +  testpmd> flow pattern_template 0 create ingress pattern_template_id 3
> +                template eth / ipv4 / udp src mask 0xffff / end
> +  testpmd> flow actions_template 0 create ingress actions_template_id 7
> +                template count  / rss / end
> +  testpmd> flow template_table 0 create table_id 101 resizable ingress
> +                group 1 priority 0 rules_number 1
> +                pattern_template 3 actions_template 7
> +
> +  # 2. Queue a flow rule.
> +  testpmd> flow queue 0 create 0 template_table 101
> +                pattern_template 0 actions_template 0 postpone no
> +                pattern eth / ipv4 / udp src spec 1 / end actions count / rss / end
> +
> +  # 3. Resize the template table
> +  #    The new table capacity is 32 rules
> +  testpmd> flow template_table 0 resize table_resize_id 101

Why not just "resize table 101" ?

> +                table_resize_rules_num 32
> +
> +  # 4. Queue more flow rules.
> +  testpmd> flow queue 0 create 0 template_table 101
> +                pattern_template 0 actions_template 0 postpone no
> +                pattern eth / ipv4 / udp src spec 2 / end actions count / rss / end
> +  testpmd> flow queue 0 create 0 template_table 101
> +                pattern_template 0 actions_template 0 postpone no
> +                pattern eth / ipv4 / udp src spec 3 / end actions count / rss / end
> +  testpmd> flow queue 0 create 0 template_table 101
> +                pattern_template 0 actions_template 0 postpone no
> +                pattern eth / ipv4 / udp src spec 4 / end actions count / rss / end
> +
> +  # 5. Queue the initial flow update.
> +  testpmd> flow queue 0 update_resized 0 rule 0
> +
> +  # 6. Complete the table resize.
> +  testpmd> flow template_table 0 resize_complete table 101
> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> index 6f8ad27808..047664a079 100644
> --- a/doc/guides/rel_notes/release_24_03.rst
> +++ b/doc/guides/rel_notes/release_24_03.rst
> @@ -55,6 +55,8 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Added support for template API table resize.**

Would be better to give a quick description to say it is adding
a new table creation flag and 4 new functions.

[...]
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> +/**
> + * Specialize table for resize.
> + */
> +#define RTE_FLOW_TABLE_SPECIALIZE_RESIZABLE RTE_BIT32(2)
>  /**@}*/
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Query whether a table can be resized.
> + *
> + * @param port_id
> + *    Port identifier of Ethernet device.
> + * @param tbl_attr
> + *    Template table.
> + *
> + * @return
> + *   True if the table can be resized.

This is not aligned, probably because of parameters above.

> + */
> +__rte_experimental
> +bool
> +rte_flow_table_resizable(__rte_unused uint16_t port_id,
> +			 const struct rte_flow_template_table_attr *tbl_attr);
> +
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Change template table flow rules capacity.
> + * PMD implementation must support table change to the new size.

I don't think it is the good place for this PMD comment.

> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param table
> + *   Template table to modify.
> + * @param nb_rules
> + *   New flow rules capacity.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) if *table* cannot be resized or resize to *nb_rules*
> + *               is not supported in PMD.
> + */
> +__rte_experimental
> +int
> +rte_flow_template_table_resize(uint16_t port_id,
> +			       struct rte_flow_template_table *table,
> +			       uint32_t nb_rules,
> +			       struct rte_flow_error *error);
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Following table resize, update flow resources in port.

You need to give details here about why, when to do it,
and what are the constraints.
It is really better to have all details in doxygen.
The RST is more to explain the global flow of the process.

> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param queue
> + *   Flow queue for async operation.
> + * @param attr
> + *   Async operation attributes.
> + * @param rule
> + *   Flow rule to update.
> + * @param user_data
> + *   The user data that will be returned on async completion event.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) if *rule* cannot be updated.
> + */
> +__rte_experimental
> +int
> +rte_flow_async_update_resized(uint16_t port_id, uint32_t queue,
> +			      const struct rte_flow_op_attr *attr,
> +			      struct rte_flow *rule, void *user_data,
> +			      struct rte_flow_error *error);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Following table resize, notify port that all table flows were updated.

You need to give details here about when to call it.

> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param table
> + *   Template table that undergoing resize operation.
> + * @param error
> + *   Perform verbose error reporting if not NULL.
> + *   PMDs initialize this structure in case of error only.
> + *
> + * @return
> + *   - (0) if success.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-ENOTSUP) if underlying device does not support this functionality.
> + *   - (-EINVAL) PMD cannot complete table resize.
> + */
> +__rte_experimental
> +int
> +rte_flow_template_table_resize_complete(uint16_t port_id,
> +					struct rte_flow_template_table *table,
> +					struct rte_flow_error *error);
[...]
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> +	# added in 24.03
> +	rte_flow_table_resizable;
> +	rte_flow_template_table_resize;
> +	rte_flow_async_update_resized;
> +	rte_flow_template_table_resize_complete;

alphabetical ordering here please