On Friday, February 11, 2022 5:17 Andrew Rybchenko
<andrew.rybche...@oktetlabs.ru> wrote:
> Sent: Friday, February 11, 2022 5:17
> To: Alexander Kozyrev <akozy...@nvidia.com>; dev@dpdk.org
> Cc: Ori Kam <or...@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <tho...@monjalon.net>; ivan.ma...@oktetlabs.ru; ferruh.yi...@intel.com;
> mohammad.abdul.a...@intel.com; qi.z.zh...@intel.com; jer...@marvell.com;
> ajit.khapa...@broadcom.com; bruce.richard...@intel.com
> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints
>
> On 2/11/22 05:26, Alexander Kozyrev wrote:
> > The flow rules creation/destruction at a large scale incurs a performance
> > penalty and may negatively impact the packet processing when used
> > as part of the datapath logic. This is mainly because software/hardware
> > resources are allocated and prepared during the flow rule creation.
> >
> > In order to optimize the insertion rate, PMD may use some hints provided
> > by the application at the initialization phase. The rte_flow_configure()
> > function allows to pre-allocate all the needed resources beforehand.
> > These resources can be used at a later stage without costly allocations.
> > Every PMD may use only the subset of hints and ignore unused ones or
> > fail in case the requested configuration is not supported.
> >
> > The rte_flow_info_get() is available to retrieve the information about
> > supported pre-configurable resources. Both these functions must be called
> > before any other usage of the flow API engine.
> >
> > Signed-off-by: Alexander Kozyrev <akozy...@nvidia.com>
> > Acked-by: Ori Kam <or...@nvidia.com>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 37 +++++++++
> > doc/guides/rel_notes/release_22_03.rst | 6 ++
> > lib/ethdev/rte_flow.c | 40 +++++++++
> > lib/ethdev/rte_flow.h | 108 +++++++++++++++++++++++++
> > lib/ethdev/rte_flow_driver.h | 10 +++
> > lib/ethdev/version.map | 2 +
> > 6 files changed, 203 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index b4aa9c47c2..72fb1132ac 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -3589,6 +3589,43 @@ Return values:
> >
> > - 0 on success, a negative errno value otherwise and ``rte_errno`` is set.
> >
> > +Flow engine configuration
> > +-------------------------
> > +
> > +Configure flow API management.
> > +
> > +An application may provide some parameters at the initialization phase
> > about
> > +rules engine configuration and/or expected flow rules characteristics.
> > +These parameters may be used by PMD to preallocate resources and
> configure NIC.
> > +
> > +Configuration
> > +~~~~~~~~~~~~~
> > +
> > +This function performs the flow API management configuration and
> > +pre-allocates needed resources beforehand to avoid costly allocations
> > later.
> > +Expected number of counters or meters in an application, for example,
> > +allow PMD to prepare and optimize NIC memory layout in advance.
> > +``rte_flow_configure()`` must be called before any flow rule is created,
> > +but after an Ethernet device is configured.
> > +
> > +.. code-block:: c
> > +
> > + int
> > + rte_flow_configure(uint16_t port_id,
> > + const struct rte_flow_port_attr *port_attr,
> > + struct rte_flow_error *error);
> > +
> > +Information about resources that can benefit from pre-allocation can be
> > +retrieved via ``rte_flow_info_get()`` API. It returns the maximum number
> > +of pre-configurable resources for a given port on a system.
> > +
> > +.. code-block:: c
> > +
> > + int
> > + rte_flow_info_get(uint16_t port_id,
> > + struct rte_flow_port_info *port_info,
> > + struct rte_flow_error *error);
> > +
> > .. _flow_isolated_mode:
> >
> > Flow isolated mode
> > diff --git a/doc/guides/rel_notes/release_22_03.rst
> b/doc/guides/rel_notes/release_22_03.rst
> > index f03183ee86..2a47a37f0a 100644
> > --- a/doc/guides/rel_notes/release_22_03.rst
> > +++ b/doc/guides/rel_notes/release_22_03.rst
> > @@ -69,6 +69,12 @@ New Features
> > New APIs, ``rte_eth_dev_priority_flow_ctrl_queue_info_get()`` and
> > ``rte_eth_dev_priority_flow_ctrl_queue_configure()``, was added.
> >
> > +* ** Added functions to configure Flow API engine
> > +
> > + * ethdev: Added ``rte_flow_configure`` API to configure Flow Management
> > + engine, allowing to pre-allocate some resources for better performance.
> > + Added ``rte_flow_info_get`` API to retrieve pre-configurable resources.
> > +
> > * **Updated AF_XDP PMD**
> >
> > * Added support for libxdp >=v1.2.2.
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index a93f68abbc..66614ae29b 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
> > ret = ops->flex_item_release(dev, handle, error);
> > return flow_err(port_id, ret, error);
> > }
> > +
> > +int
> > +rte_flow_info_get(uint16_t port_id,
> > + struct rte_flow_port_info *port_info,
> > + struct rte_flow_error *error)
> > +{
> > + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> > +
> > + if (unlikely(!ops))
> > + return -rte_errno;
> > + if (likely(!!ops->info_get)) {
>
> expected ethdev state must be validated. Just configured?
>
> > + return flow_err(port_id,
> > + ops->info_get(dev, port_info, error),
>
> port_info must be checked vs NULL
We don’t have any NULL checks for parameters in the whole ret flow API library.
See rte_flow_create() for example. attributes, pattern and actions are passed
to PMD unchecked.
> > + error);
> > + }
> > + return rte_flow_error_set(error, ENOTSUP,
> > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > + NULL, rte_strerror(ENOTSUP));
> > +}
> > +
> > +int
> > +rte_flow_configure(uint16_t port_id,
> > + const struct rte_flow_port_attr *port_attr,
> > + struct rte_flow_error *error)
> > +{
> > + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> > +
> > + if (unlikely(!ops))
> > + return -rte_errno;
> > + if (likely(!!ops->configure)) {
>
> The API must validate ethdev state. configured and not started?
Again, we have no such validation for any rte flow API today.
>
> > + return flow_err(port_id,
> > + ops->configure(dev, port_attr, error),
>
> port_attr must be checked vs NULL
Same.
> > + error);
> > + }
> > + return rte_flow_error_set(error, ENOTSUP,
> > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > + NULL, rte_strerror(ENOTSUP));
> > +}
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 1031fb246b..92be2a9a89 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t port_id,
> > const struct rte_flow_item_flex_handle *handle,
> > struct rte_flow_error *error);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Information about available pre-configurable resources.
> > + * The zero value means a resource cannot be pre-allocated.
> > + *
> > + */
> > +struct rte_flow_port_info {
> > + /**
> > + * Number of pre-configurable counter actions.
> > + * @see RTE_FLOW_ACTION_TYPE_COUNT
> > + */
> > + uint32_t nb_counters;
>
> Name says that it is a number of counters, but description
> says that it is about actions.
> Also I don't understand what does "pre-configurable" mean.
> Isn't it a maximum number of available counters?
> If no, how can I find a maximum?
It is number of pre-allocated and pre-configured actions.
How are they pr-configured is up to PDM driver.
But let's change to "pre-configured" everywhere.
Configuration includes some memory allocation anyway.
>
> > + /**
> > + * Number of pre-configurable aging flows actions.
> > + * @see RTE_FLOW_ACTION_TYPE_AGE
> > + */
> > + uint32_t nb_aging_flows;
>
> Same
Ditto.
> > + /**
> > + * Number of pre-configurable traffic metering actions.
> > + * @see RTE_FLOW_ACTION_TYPE_METER
> > + */
> > + uint32_t nb_meters;
>
> Same
Ditto.
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Retrieve configuration attributes supported by the port.
>
> Description should be a bit more flow API aware.
> Right now it sounds too generic.
Ok, how about
"Get information about flow engine pre-configurable resources."
> > + *
> > + * @param port_id
> > + * Port identifier of Ethernet device.
> > + * @param[out] port_info
> > + * A pointer to a structure of type *rte_flow_port_info*
> > + * to be filled with the contextual information of the port.
> > + * @param[out] error
> > + * Perform verbose error reporting if not NULL.
> > + * PMDs initialize this structure in case of error only.
> > + *
> > + * @return
> > + * 0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_info_get(uint16_t port_id,
> > + struct rte_flow_port_info *port_info,
> > + struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Resource pre-allocation and pre-configuration settings.
>
> What is the difference between pre-allocation and pre-configuration?
> Why are both mentioned above, but just pre-configured actions are
> mentioned below?
Please see answer to this question above.
> > + * The zero value means on demand resource allocations only.
> > + *
> > + */
> > +struct rte_flow_port_attr {
> > + /**
> > + * Number of counter actions pre-configured.
> > + * @see RTE_FLOW_ACTION_TYPE_COUNT
> > + */
> > + uint32_t nb_counters;
> > + /**
> > + * Number of aging flows actions pre-configured.
> > + * @see RTE_FLOW_ACTION_TYPE_AGE
> > + */
> > + uint32_t nb_aging_flows;
> > + /**
> > + * Number of traffic metering actions pre-configured.
> > + * @see RTE_FLOW_ACTION_TYPE_METER
> > + */
> > + uint32_t nb_meters;
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Configure the port's flow API engine.
> > + *
> > + * This API can only be invoked before the application
> > + * starts using the rest of the flow library functions.
> > + *
> > + * The API can be invoked multiple times to change the
> > + * settings. The port, however, may reject the changes.
> > + *
> > + * Parameters in configuration attributes must not exceed
> > + * numbers of resources returned by the rte_flow_info_get API.
> > + *
> > + * @param port_id
> > + * Port identifier of Ethernet device.
> > + * @param[in] port_attr
> > + * Port configuration attributes.
> > + * @param[out] error
> > + * Perform verbose error reporting if not NULL.
> > + * PMDs initialize this structure in case of error only.
> > + *
> > + * @return
> > + * 0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_configure(uint16_t port_id,
> > + const struct rte_flow_port_attr *port_attr,
> > + struct rte_flow_error *error);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h
> > index f691b04af4..7c29930d0f 100644
> > --- a/lib/ethdev/rte_flow_driver.h
> > +++ b/lib/ethdev/rte_flow_driver.h
> > @@ -152,6 +152,16 @@ struct rte_flow_ops {
> > (struct rte_eth_dev *dev,
> > const struct rte_flow_item_flex_handle *handle,
> > struct rte_flow_error *error);
> > + /** See rte_flow_info_get() */
> > + int (*info_get)
> > + (struct rte_eth_dev *dev,
> > + struct rte_flow_port_info *port_info,
> > + struct rte_flow_error *err);
> > + /** See rte_flow_configure() */
> > + int (*configure)
> > + (struct rte_eth_dev *dev,
> > + const struct rte_flow_port_attr *port_attr,
> > + struct rte_flow_error *err);
> > };
> >
> > /**
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index cd0c4c428d..f1235aa913 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -260,6 +260,8 @@ EXPERIMENTAL {
> > # added in 22.03
> > rte_eth_dev_priority_flow_ctrl_queue_configure;
> > rte_eth_dev_priority_flow_ctrl_queue_info_get;
> > + rte_flow_info_get;
> > + rte_flow_configure;
> > };
> >
> > INTERNAL {
