Re: [dpdk-dev] [PATCH 4/4] ethdev: document indirect flow action life cycle

[Ori Kam <orika@nvidia.com>]

Hi Dmitry,

> -----Original Message-----
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Tuesday, July 27, 2021 10:31 AM
>
> Subject: [PATCH 4/4] ethdev: document indirect flow action life cycle
> 
> rte_flow_action_handle_create() did not specify what happens with an
> indirect action when device is stopped, possibly reconfigured, and started
> again.
> 
> It is proposed that indirect actions persisted across such a sequence.
> This allows for easier API usage and better HW resources utilization by saving
> indirect actions flush and re-creation with associated error handling and
> rollback.
> 
> If between stop and start a device is reconfigured in a way that is
> incompatible with an existing indirect action, PMD is required to report an
> error at the device start. This is mandatory, because flow API does not supply
> users with capabilities, so this is the only way for a user to learn that
> configuration is invalid. Errors are not reported at configuration stage to give
> the user a chance to remove or change offending actions. For example, if
> number of queues changes and an RSS indirect action specifies queues that
> went away, user must update the action before starting the device. PMD is
> not allowed to silently adjust indirect actions (in the same example, to
> remove queues from the RSS), so that all configuration is explicit.
> 
I think some of the errors can be checked during configuration.
For example when app removes a queue the PMD can check if there is any
reference on this queue and fail at this point.

> Fixes: 4b61b8774be9 ("ethdev: introduce indirect flow action")
> Cc: bingz@nvidia.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
>  lib/ethdev/rte_flow.h              |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 2b42d5ec8c..06dd06d9a6 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2785,6 +2785,16 @@ updated depend on the type of the ``action`` and
> different for every type.
>  The indirect action specified data (e.g. counter) can be queried by
> ``rte_flow_action_handle_query()``.
> 
> +Indirect actions persist across device configure, stop, and start.
+1
> +If a new configuration is incompatible with an existing indirect
> +action, the start operation will fail. "Incompatible" means that if
> +this action was destroyed and created again, creation would fail.

I think this test should be done during the configuration.
It is the application responsibility to use valid actions, it should be stated
that changing the values of configuration may result in in valid actions.

> +It is a programmer's responsibility to remove or update offending actions.
> +
> +PMD developers should use the same diagnostics for
> +``rte_eth_dev_start()`` as for ``rte_flow_action_handle_create()``. PMD
> +is not allowed to silently ignore or correct offending actions.
> +
>  .. _table_rte_flow_action_handle:
> 
>  .. table:: INDIRECT
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> 70f455d47d..f571a27fe7 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3969,6 +3969,10 @@ struct rte_flow_indir_action_conf {
>   * The created object handle has single state and configuration
>   * across all the flow rules using it.
>   *
> + * Indirect actions persist across device configure, stop, and start.
> + * If a new configuration is incompatible with an existing indirect
> + * action, rte_eth_dev_start() will fail.
> + *

I don't think that dev start should check configuration it may slow down startup.

>   * @param[in] port_id
>   *    The port identifier of the Ethernet device.
>   * @param[in] conf
> --
> 2.25.1

Thanks,
Ori