Re: [dpdk-dev] [PATCH v2 2/5] ethdev: add capability to keep shared objects on restart

[Ori Kam <orika@nvidia.com>]

Hi Dmitry

> -----Original Message-----
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Friday, October 15, 2021 7:18 PM
> To: dev@dpdk.org
> Subject: [PATCH v2 2/5] ethdev: add capability to keep shared objects on restart
> 
> rte_flow_action_handle_create() did not mention what happens with an indirect action when a
> device is stopped, possibly reconfigured, and started again. It is natural for some indirect actions to be
> persistent, like counters and meters; keeping others just saves application time and complexity.
> However, not all PMDs can support it.
> Also the support may be limited by particular action kinds, that is, combinations of action type and the
> value of the transfer bit in its configuration.
> 
> Add a device capability to indicate if at least some indirect actions are kept across the above sequence.
> Without this capability the behavior is still unspecified, but now it is stated explicitly.
> In the future, indirect actions may not be the only type of objects shared between flow rules. The
> capability bit intends to cover all possible types of such objects, hence its name.
> 
> Declare that the application can test for the persistence of a particular indirect action kind by
> attempting to create an indirect action of that kind when the device is stopped and checking for the
> specific error type.
> This is logical because if the PMD can to create the flow rule when the device is not started and use it
> after the start happens, it is natural that it can move its internal flow shared object to the same state
> when the device is stopped and restore the state when the device is started.
> 
> If the device is being reconfigured in a way that is incompatible with an existing shared objects, PMD is
> required to report an error.
> 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. For example, if queue count changes and RSS indirect
> action specifies queues that are going away, the user must update the action before removing the
> queues or remove the action and all flow rules that were using it.
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 24 ++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.h            |  3 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index b0ced4209b..bf96ad830f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2812,6 +2812,30 @@ 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()``.
> 
> +By default it is unspecified if indirect actions persist after the device stop.
> +If ``RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP`` is not advertised, then
> +indirect actions must be explicitly destroyed before stopping the
> +device if the application needs to ensure they are removed.

I don't understand the above line, if indirect actions must be explicitly destroyed
what does it means "if the application needs to ensure?)
I think it should say just like now, application should destroy all shared objects before
stopping. If application doesn't call the destroy the state of the action is undefined.


> +If it is advertised, this means the PMD can keep at least some indirect
> +actions across device stop and start with possible reconfiguration in between.
> +However, it may be only supported for certain kinds of indirect actions.
> +The kind is a combination of the action type and the value of its transfer bit.
> +To test if a particular kind of indirect actions is kept, the
> +application must try to create a valid indirect action of that kind
> +when the device is stopped (after it has been configured or started previously).
> +If it succeeds, all indirect actions of the same kind are kept when the
> +device is stopped.
> +If it fails with an error of type ``RTE_FLOW_ERROR_TYPE_STATE``,
> +indirect actions of this kind are flushed when the device is stopped.
> +Indirect actions of a kept kind that are created when the device is
> +stopped, including the ones created for the test, will be kept after the device start.
> +Some configuration changes may be incompatible with existing indirect actions.
> +In this case ``rte_eth_dev_configure()``,
> +``rte_eth_rx/tx_queue_setup()``, and/or ``rte_eth_dev_start()`` will
> +fail with a log message from the PMD that should be similar to the one
> +that would be emitted by ``rte_flow_action_handle_create()`` if an
> +attempt was made to create the offending rule with the new configuration.
> +
>  .. _table_rte_flow_action_handle:
> 
>  .. table:: INDIRECT
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index a0b388bb25..12fc7262eb 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -94,6 +94,7 @@
>   * depending on the device capabilities:
>   *
>   *     - flow rules
> + *     - flow-related shared objects, e.g. indirect actions
>   *
>   * Any other configuration will not be stored and will need to be re-entered
>   * before a call to rte_eth_dev_start().
> @@ -1452,6 +1453,8 @@ struct rte_eth_conf {  #define
> RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP 0x00000002
>  /** Device supports keeping flow rules across restart. */  #define
> RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP 0x00000004
> +/** Device supports keeping shared flow objects across restart. */
> +#define RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP 0x00000008
>  /**@}*/
> 
>  /*
> --
> 2.25.1

Best,
Ori