[v8,01/11] ethdev: introduce flow engine configuration
Checks
Commit Message
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 <akozyrev@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
doc/guides/prog_guide/rte_flow.rst | 36 ++++++++
doc/guides/rel_notes/release_22_03.rst | 6 ++
lib/ethdev/ethdev_driver.h | 7 +-
lib/ethdev/rte_flow.c | 69 +++++++++++++++
lib/ethdev/rte_flow.h | 111 +++++++++++++++++++++++++
lib/ethdev/rte_flow_driver.h | 10 +++
lib/ethdev/version.map | 2 +
7 files changed, 240 insertions(+), 1 deletion(-)
Comments
On 2/20/22 06:43, 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 <akozyrev@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
[snip]
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 6d697a879a..06f0896e1e 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -138,7 +138,12 @@ struct rte_eth_dev_data {
> * Indicates whether the device is configured:
> * CONFIGURED(1) / NOT CONFIGURED(0)
> */
> - dev_configured : 1;
> + dev_configured:1,
Above is unrelated to the patch. Moreover, it breaks style used
few lines above.
> + /**
> + * Indicates whether the flow engine is configured:
> + * CONFIGURED(1) / NOT CONFIGURED(0)
> + */
> + flow_configured:1;
I'd like to understand why we need the information. It is
unclear from the patch. Right now it is write-only. Nobody
checks it. Is flow engine configuration become a mandatory
step? Always? Just in some cases?
>
> /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 7f93900bc8..ffd48e40d5 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> + return -EINVAL;
> + }
> + if (dev->data->dev_configured == 0) {
> + RTE_FLOW_LOG(INFO,
> + "Device with port_id=%"PRIu16" is not configured.\n",
> + port_id);
> + return -EINVAL;
> + }
> + if (unlikely(!ops))
> + return -rte_errno;
Order of checks is not always obvious, but requires at
least some rules to follow. When there is no any good
reason to do otherwise, I'd suggest to check arguments
in there order. I.e. check port_id and its direct
derivatives first:
1. ops (since it is NULL if port_id is invalid)
2. dev_configured (since only port_id is required to check it)
3. port_info (since it goes after port_id)
> + if (likely(!!ops->info_get)) {
> + return flow_err(port_id,
> + ops->info_get(dev, port_info, error),
> + 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);
> + int ret;
> +
> + dev->data->flow_configured = 0;
> + if (port_attr == NULL) {
> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> + return -EINVAL;
> + }
> + if (dev->data->dev_configured == 0) {
> + RTE_FLOW_LOG(INFO,
> + "Device with port_id=%"PRIu16" is not configured.\n",
> + port_id);
> + return -EINVAL;
> + }
> + if (dev->data->dev_started != 0) {
> + RTE_FLOW_LOG(INFO,
> + "Device with port_id=%"PRIu16" already started.\n",
> + port_id);
> + return -EINVAL;
> + }
> + if (unlikely(!ops))
> + return -rte_errno;
Same logic here:
1. ops
2. dev_configured
3. dev_started
4. port_attr
5. ops->configure since we want to be sure that state and input
arguments are valid before calling it
> + if (likely(!!ops->configure)) {
> + ret = ops->configure(dev, port_attr, error);
> + if (ret == 0)
> + dev->data->flow_configured = 1;
> + return flow_err(port_id, ret, error);
> + }
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, rte_strerror(ENOTSUP));
> +}
[snip]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Get information about flow engine 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 resources 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.
If I'm not mistakes we should be explicit with
negative result values menting
> + */
> +__rte_experimental
> +int
> +rte_flow_info_get(uint16_t port_id,
> + struct rte_flow_port_info *port_info,
> + struct rte_flow_error *error);
[snip]
> +/**
> + * @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.
Same here.
[snip]
On 2/21/22 12:47, Andrew Rybchenko wrote:
> On 2/20/22 06:43, 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 <akozyrev@nvidia.com>
>> Acked-by: Ori Kam <orika@nvidia.com>
>
> [snip]
>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 6d697a879a..06f0896e1e 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -138,7 +138,12 @@ struct rte_eth_dev_data {
>> * Indicates whether the device is configured:
>> * CONFIGURED(1) / NOT CONFIGURED(0)
>> */
>> - dev_configured : 1;
>> + dev_configured:1,
>
> Above is unrelated to the patch. Moreover, it breaks style used
> few lines above.
>
>> + /**
>> + * Indicates whether the flow engine is configured:
>> + * CONFIGURED(1) / NOT CONFIGURED(0)
>> + */
>> + flow_configured:1;
>
> I'd like to understand why we need the information. It is
> unclear from the patch. Right now it is write-only. Nobody
> checks it. Is flow engine configuration become a mandatory
> step? Always? Just in some cases?
>
>> /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>> index 7f93900bc8..ffd48e40d5 100644
>> --- a/lib/ethdev/rte_flow.c
>> +++ b/lib/ethdev/rte_flow.c
>> @@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
>> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
>> + return -EINVAL;
>> + }
>> + if (dev->data->dev_configured == 0) {
>> + RTE_FLOW_LOG(INFO,
>> + "Device with port_id=%"PRIu16" is not configured.\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> + if (unlikely(!ops))
>> + return -rte_errno;
>
> Order of checks is not always obvious, but requires at
> least some rules to follow. When there is no any good
> reason to do otherwise, I'd suggest to check arguments
> in there order. I.e. check port_id and its direct
> derivatives first:
> 1. ops (since it is NULL if port_id is invalid)
> 2. dev_configured (since only port_id is required to check it)
> 3. port_info (since it goes after port_id)
>
>> + if (likely(!!ops->info_get)) {
>> + return flow_err(port_id,
>> + ops->info_get(dev, port_info, error),
>> + 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);
>> + int ret;
>> +
>> + dev->data->flow_configured = 0;
>> + if (port_attr == NULL) {
>> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
>> + return -EINVAL;
>> + }
>> + if (dev->data->dev_configured == 0) {
>> + RTE_FLOW_LOG(INFO,
>> + "Device with port_id=%"PRIu16" is not configured.\n",
>> + port_id);
>> + return -EINVAL;
>> + }
In fact there is one more interesting question related
to device states. Necessity to call flow info and flow
configure in configured state allows configure to rely
on device configuration. The question is: what should
happen with the device flow engine configuration if
the device is reconfigured?
>> + if (dev->data->dev_started != 0) {
>> + RTE_FLOW_LOG(INFO,
>> + "Device with port_id=%"PRIu16" already started.\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> + if (unlikely(!ops))
>> + return -rte_errno;
>
> Same logic here:
> 1. ops
> 2. dev_configured
> 3. dev_started
> 4. port_attr
> 5. ops->configure since we want to be sure that state and input
> arguments are valid before calling it
>
>> + if (likely(!!ops->configure)) {
>> + ret = ops->configure(dev, port_attr, error);
>> + if (ret == 0)
>> + dev->data->flow_configured = 1;
>> + return flow_err(port_id, ret, error);
>> + }
>> + return rte_flow_error_set(error, ENOTSUP,
>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>> + NULL, rte_strerror(ENOTSUP));
>> +}
>
> [snip]
>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Get information about flow engine 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 resources 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.
>
> If I'm not mistakes we should be explicit with
> negative result values menting
>
>> + */
>> +__rte_experimental
>> +int
>> +rte_flow_info_get(uint16_t port_id,
>> + struct rte_flow_port_info *port_info,
>> + struct rte_flow_error *error);
>
> [snip]
>
>> +/**
>> + * @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.
>
> Same here.
>
> [snip]
Hi Andrew and Alexander,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, February 21, 2022 11:53 AM
> Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration
>
> On 2/21/22 12:47, Andrew Rybchenko wrote:
> > On 2/20/22 06:43, 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 <akozyrev@nvidia.com>
> >> Acked-by: Ori Kam <orika@nvidia.com>
> >
> > [snip]
> >
> >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> >> index 6d697a879a..06f0896e1e 100644
> >> --- a/lib/ethdev/ethdev_driver.h
> >> +++ b/lib/ethdev/ethdev_driver.h
> >> @@ -138,7 +138,12 @@ struct rte_eth_dev_data {
> >> * Indicates whether the device is configured:
> >> * CONFIGURED(1) / NOT CONFIGURED(0)
> >> */
> >> - dev_configured : 1;
> >> + dev_configured:1,
> >
> > Above is unrelated to the patch. Moreover, it breaks style used
> > few lines above.
> >
+1
> >> + /**
> >> + * Indicates whether the flow engine is configured:
> >> + * CONFIGURED(1) / NOT CONFIGURED(0)
> >> + */
> >> + flow_configured:1;
> >
> > I'd like to understand why we need the information. It is
> > unclear from the patch. Right now it is write-only. Nobody
> > checks it. Is flow engine configuration become a mandatory
> > step? Always? Just in some cases?
> >
See my commets below,
I can see two ways or remove this member or check in each control function
that the configuration function was done.
> >> /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
> >> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> >> index 7f93900bc8..ffd48e40d5 100644
> >> --- a/lib/ethdev/rte_flow.c
> >> +++ b/lib/ethdev/rte_flow.c
> >> @@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
> >> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> >> + return -EINVAL;
> >> + }
> >> + if (dev->data->dev_configured == 0) {
> >> + RTE_FLOW_LOG(INFO,
> >> + "Device with port_id=%"PRIu16" is not configured.\n",
> >> + port_id);
> >> + return -EINVAL;
> >> + }
> >> + if (unlikely(!ops))
> >> + return -rte_errno;
> >
> > Order of checks is not always obvious, but requires at
> > least some rules to follow. When there is no any good
> > reason to do otherwise, I'd suggest to check arguments
> > in there order. I.e. check port_id and its direct
> > derivatives first:
> > 1. ops (since it is NULL if port_id is invalid)
> > 2. dev_configured (since only port_id is required to check it)
> > 3. port_info (since it goes after port_id)
> >
Agree,
> >> + if (likely(!!ops->info_get)) {
> >> + return flow_err(port_id,
> >> + ops->info_get(dev, port_info, error),
> >> + 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);
> >> + int ret;
> >> +
> >> + dev->data->flow_configured = 0;
I don't think there is meaning to add this set here.
I would remove this field.
Unless you want to check it for all control functions.
> >> + if (port_attr == NULL) {
> >> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> >> + return -EINVAL;
> >> + }
> >> + if (dev->data->dev_configured == 0) {
> >> + RTE_FLOW_LOG(INFO,
> >> + "Device with port_id=%"PRIu16" is not configured.\n",
> >> + port_id);
> >> + return -EINVAL;
> >> + }
>
> In fact there is one more interesting question related
> to device states. Necessity to call flow info and flow
> configure in configured state allows configure to rely
> on device configuration. The question is: what should
> happen with the device flow engine configuration if
> the device is reconfigured?
>
That’s dependes on PMD.
PMD may support reconfiguring, partial reconfigure (for example only number of objects
but not changing the number of queues) or it will not support any reconfigure.
It may also be dependent if the port is started or not.
Currently we don't plan to support reconfigure but in future we may support changing the
number of objects.
> >> + if (dev->data->dev_started != 0) {
> >> + RTE_FLOW_LOG(INFO,
> >> + "Device with port_id=%"PRIu16" already started.\n",
> >> + port_id);
> >> + return -EINVAL;
> >> + }
> >> + if (unlikely(!ops))
> >> + return -rte_errno;
> >
> > Same logic here:
> > 1. ops
> > 2. dev_configured
> > 3. dev_started
> > 4. port_attr
> > 5. ops->configure since we want to be sure that state and input
> > arguments are valid before calling it
> >
> >> + if (likely(!!ops->configure)) {
> >> + ret = ops->configure(dev, port_attr, error);
> >> + if (ret == 0)
> >> + dev->data->flow_configured = 1;
> >> + return flow_err(port_id, ret, error);
> >> + }
> >> + return rte_flow_error_set(error, ENOTSUP,
> >> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >> + NULL, rte_strerror(ENOTSUP));
> >> +}
> >
> > [snip]
> >
> >> +/**
> >> + * @warning
> >> + * @b EXPERIMENTAL: this API may change without prior notice.
> >> + *
> >> + * Get information about flow engine 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 resources 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.
> >
> > If I'm not mistakes we should be explicit with
> > negative result values menting
> >
I'm not sure, until now we didn't have any errors values defined in RTE flow.
I don't want to enforce PMD with the error types.
If PMD can say that it can give better error code or add a case that may result in
error, I don't want to change the API.
So I think we better leave the error codes out of documentation unless they are final and can only
be resulted from the rte_level.
> >> + */
> >> +__rte_experimental
> >> +int
> >> +rte_flow_info_get(uint16_t port_id,
> >> + struct rte_flow_port_info *port_info,
> >> + struct rte_flow_error *error);
> >
> > [snip]
> >
> >> +/**
> >> + * @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.
> >
> > Same here.
> >
Same as above.
> > [snip]
Best,
ORi
On Monday, February 21, 2022 7:54 Ori Kam <orika@nvidia.com> wrote:
> Hi Andrew and Alexander,
>
> > -----Original Message-----
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Sent: Monday, February 21, 2022 11:53 AM
> > Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration
> >
> > On 2/21/22 12:47, Andrew Rybchenko wrote:
> > > On 2/20/22 06:43, 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 <akozyrev@nvidia.com>
> > >> Acked-by: Ori Kam <orika@nvidia.com>
> > >
> > > [snip]
> > >
> > >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > >> index 6d697a879a..06f0896e1e 100644
> > >> --- a/lib/ethdev/ethdev_driver.h
> > >> +++ b/lib/ethdev/ethdev_driver.h
> > >> @@ -138,7 +138,12 @@ struct rte_eth_dev_data {
> > >> * Indicates whether the device is configured:
> > >> * CONFIGURED(1) / NOT CONFIGURED(0)
> > >> */
> > >> - dev_configured : 1;
> > >> + dev_configured:1,
> > >
> > > Above is unrelated to the patch. Moreover, it breaks style used
> > > few lines above.
> > >
> +1
It is related, I had to change this line to add flow_configured member.
And there is a waring if I keep old style:
ERROR:SPACING: space prohibited before that ':' (ctx:WxW)
Should I keep the old style with warnings or change all members to fix it?
> > >> + /**
> > >> + * Indicates whether the flow engine is configured:
> > >> + * CONFIGURED(1) / NOT CONFIGURED(0)
> > >> + */
> > >> + flow_configured:1;
> > >
> > > I'd like to understand why we need the information. It is
> > > unclear from the patch. Right now it is write-only. Nobody
> > > checks it. Is flow engine configuration become a mandatory
> > > step? Always? Just in some cases?
> > >
>
> See my commets below,
> I can see two ways or remove this member or check in each control function
> that the configuration function was done.
It is write-only in this patch, rte_flow_configure() sets it when configuration is done.
The it is checked in the templates/tables creation in patch 2.
We do not allow tamplates/tables creation without invoking configure first.
> > >> /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
> > >> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> > >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > >> index 7f93900bc8..ffd48e40d5 100644
> > >> --- a/lib/ethdev/rte_flow.c
> > >> +++ b/lib/ethdev/rte_flow.c
> > >> @@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
> > >> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> > >> + return -EINVAL;
> > >> + }
> > >> + if (dev->data->dev_configured == 0) {
> > >> + RTE_FLOW_LOG(INFO,
> > >> + "Device with port_id=%"PRIu16" is not configured.\n",
> > >> + port_id);
> > >> + return -EINVAL;
> > >> + }
> > >> + if (unlikely(!ops))
> > >> + return -rte_errno;
> > >
> > > Order of checks is not always obvious, but requires at
> > > least some rules to follow. When there is no any good
> > > reason to do otherwise, I'd suggest to check arguments
> > > in there order. I.e. check port_id and its direct
> > > derivatives first:
> > > 1. ops (since it is NULL if port_id is invalid)
> > > 2. dev_configured (since only port_id is required to check it)
> > > 3. port_info (since it goes after port_id)
> > >
>
> Agree,
Ok.
> > >> + if (likely(!!ops->info_get)) {
> > >> + return flow_err(port_id,
> > >> + ops->info_get(dev, port_info, error),
> > >> + 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);
> > >> + int ret;
> > >> +
> > >> + dev->data->flow_configured = 0;
>
> I don't think there is meaning to add this set here.
> I would remove this field.
> Unless you want to check it for all control functions.
I do check it in templates/tables creation API as I mentioned above.
> > >> + if (port_attr == NULL) {
> > >> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
> > >> + return -EINVAL;
> > >> + }
> > >> + if (dev->data->dev_configured == 0) {
> > >> + RTE_FLOW_LOG(INFO,
> > >> + "Device with port_id=%"PRIu16" is not configured.\n",
> > >> + port_id);
> > >> + return -EINVAL;
> > >> + }
> >
> > In fact there is one more interesting question related
> > to device states. Necessity to call flow info and flow
> > configure in configured state allows configure to rely
> > on device configuration. The question is: what should
> > happen with the device flow engine configuration if
> > the device is reconfigured?
> >
>
> That’s dependes on PMD.
> PMD may support reconfiguring, partial reconfigure (for example only number
> of objects
> but not changing the number of queues) or it will not support any reconfigure.
> It may also be dependent if the port is started or not.
> Currently we don't plan to support reconfigure but in future we may support
> changing the
> number of objects.
> > >> + if (dev->data->dev_started != 0) {
> > >> + RTE_FLOW_LOG(INFO,
> > >> + "Device with port_id=%"PRIu16" already started.\n",
> > >> + port_id);
> > >> + return -EINVAL;
> > >> + }
> > >> + if (unlikely(!ops))
> > >> + return -rte_errno;
> > >
> > > Same logic here:
> > > 1. ops
> > > 2. dev_configured
> > > 3. dev_started
> > > 4. port_attr
> > > 5. ops->configure since we want to be sure that state and input
> > > arguments are valid before calling it
> > >
> > >> + if (likely(!!ops->configure)) {
> > >> + ret = ops->configure(dev, port_attr, error);
> > >> + if (ret == 0)
> > >> + dev->data->flow_configured = 1;
> > >> + return flow_err(port_id, ret, error);
> > >> + }
> > >> + return rte_flow_error_set(error, ENOTSUP,
> > >> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > >> + NULL, rte_strerror(ENOTSUP));
> > >> +}
> > >
> > > [snip]
> > >
> > >> +/**
> > >> + * @warning
> > >> + * @b EXPERIMENTAL: this API may change without prior notice.
> > >> + *
> > >> + * Get information about flow engine 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 resources 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.
> > >
> > > If I'm not mistakes we should be explicit with
> > > negative result values menting
> > >
> I'm not sure, until now we didn't have any errors values defined in RTE flow.
> I don't want to enforce PMD with the error types.
> If PMD can say that it can give better error code or add a case that may result in
> error, I don't want to change the API.
> So I think we better leave the error codes out of documentation unless they are
> final and can only
> be resulted from the rte_level.
>
> > >> + */
> > >> +__rte_experimental
> > >> +int
> > >> +rte_flow_info_get(uint16_t port_id,
> > >> + struct rte_flow_port_info *port_info,
> > >> + struct rte_flow_error *error);
> > >
> > > [snip]
> > >
> > >> +/**
> > >> + * @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.
> > >
> > > Same here.
> > >
> Same as above.
>
> > > [snip]
>
> Best,
> ORi
On 2/21/22 15:53, Ori Kam wrote:
> Hi Andrew and Alexander,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, February 21, 2022 11:53 AM
>> Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration
>>
>> On 2/21/22 12:47, Andrew Rybchenko wrote:
>>> On 2/20/22 06:43, 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 <akozyrev@nvidia.com>
>>>> Acked-by: Ori Kam <orika@nvidia.com>
>>>
>>> [snip]
>>>
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index 6d697a879a..06f0896e1e 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -138,7 +138,12 @@ struct rte_eth_dev_data {
>>>> * Indicates whether the device is configured:
>>>> * CONFIGURED(1) / NOT CONFIGURED(0)
>>>> */
>>>> - dev_configured : 1;
>>>> + dev_configured:1,
>>>
>>> Above is unrelated to the patch. Moreover, it breaks style used
>>> few lines above.
>>>
> +1
>>>> + /**
>>>> + * Indicates whether the flow engine is configured:
>>>> + * CONFIGURED(1) / NOT CONFIGURED(0)
>>>> + */
>>>> + flow_configured:1;
>>>
>>> I'd like to understand why we need the information. It is
>>> unclear from the patch. Right now it is write-only. Nobody
>>> checks it. Is flow engine configuration become a mandatory
>>> step? Always? Just in some cases?
>>>
>
> See my commets below,
> I can see two ways or remove this member or check in each control function
> that the configuration function was done.
>
>>>> /** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
>>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>>>> index 7f93900bc8..ffd48e40d5 100644
>>>> --- a/lib/ethdev/rte_flow.c
>>>> +++ b/lib/ethdev/rte_flow.c
>>>> @@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
>>>> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
>>>> + return -EINVAL;
>>>> + }
>>>> + if (dev->data->dev_configured == 0) {
>>>> + RTE_FLOW_LOG(INFO,
>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>>> + port_id);
>>>> + return -EINVAL;
>>>> + }
>>>> + if (unlikely(!ops))
>>>> + return -rte_errno;
>>>
>>> Order of checks is not always obvious, but requires at
>>> least some rules to follow. When there is no any good
>>> reason to do otherwise, I'd suggest to check arguments
>>> in there order. I.e. check port_id and its direct
>>> derivatives first:
>>> 1. ops (since it is NULL if port_id is invalid)
>>> 2. dev_configured (since only port_id is required to check it)
>>> 3. port_info (since it goes after port_id)
>>>
>
> Agree,
>
>>>> + if (likely(!!ops->info_get)) {
>>>> + return flow_err(port_id,
>>>> + ops->info_get(dev, port_info, error),
>>>> + 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);
>>>> + int ret;
>>>> +
>>>> + dev->data->flow_configured = 0;
>
> I don't think there is meaning to add this set here.
> I would remove this field.
> Unless you want to check it for all control functions.
>
>>>> + if (port_attr == NULL) {
>>>> + RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
>>>> + return -EINVAL;
>>>> + }
>>>> + if (dev->data->dev_configured == 0) {
>>>> + RTE_FLOW_LOG(INFO,
>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>>> + port_id);
>>>> + return -EINVAL;
>>>> + }
>>
>> In fact there is one more interesting question related
>> to device states. Necessity to call flow info and flow
>> configure in configured state allows configure to rely
>> on device configuration. The question is: what should
>> happen with the device flow engine configuration if
>> the device is reconfigured?
>>
>
> That’s dependes on PMD.
> PMD may support reconfiguring, partial reconfigure (for example only number of objects
> but not changing the number of queues) or it will not support any reconfigure.
> It may also be dependent if the port is started or not.
> Currently we don't plan to support reconfigure but in future we may support changing the
> number of objects.
But we should define behaviour and say what application should
expect. Above sounds like: Flow engine configuration persists
across device reconfigure.
>
>>>> + if (dev->data->dev_started != 0) {
>>>> + RTE_FLOW_LOG(INFO,
>>>> + "Device with port_id=%"PRIu16" already started.\n",
>>>> + port_id);
>>>> + return -EINVAL;
>>>> + }
>>>> + if (unlikely(!ops))
>>>> + return -rte_errno;
>>>
>>> Same logic here:
>>> 1. ops
>>> 2. dev_configured
>>> 3. dev_started
>>> 4. port_attr
>>> 5. ops->configure since we want to be sure that state and input
>>> arguments are valid before calling it
>>>
>>>> + if (likely(!!ops->configure)) {
>>>> + ret = ops->configure(dev, port_attr, error);
>>>> + if (ret == 0)
>>>> + dev->data->flow_configured = 1;
>>>> + return flow_err(port_id, ret, error);
>>>> + }
>>>> + return rte_flow_error_set(error, ENOTSUP,
>>>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>>> + NULL, rte_strerror(ENOTSUP));
>>>> +}
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + * @warning
>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>> + *
>>>> + * Get information about flow engine 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 resources 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.
>>>
>>> If I'm not mistakes we should be explicit with
>>> negative result values menting
>>>
> I'm not sure, until now we didn't have any errors values defined in RTE flow.
> I don't want to enforce PMD with the error types.
> If PMD can say that it can give better error code or add a case that may result in
> error, I don't want to change the API.
> So I think we better leave the error codes out of documentation unless they are final and can only
> be resulted from the rte_level.
It is not helpful for application. If so, application don't
know how to interpret and handle various error codes.
>>>> + */
>>>> +__rte_experimental
>>>> +int
>>>> +rte_flow_info_get(uint16_t port_id,
>>>> + struct rte_flow_port_info *port_info,
>>>> + struct rte_flow_error *error);
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + * @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.
>>>
>>> Same here.
>>>
> Same as above.
>
>>> [snip]
>
> Best,
> ORi
21/02/2022 15:53, Andrew Rybchenko:
> On 2/21/22 15:53, Ori Kam wrote:
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> +/**
> >>>> + * @warning
> >>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>> + *
> >>>> + * Get information about flow engine 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 resources 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.
> >>>
> >>> If I'm not mistakes we should be explicit with
> >>> negative result values menting
> >>>
> > I'm not sure, until now we didn't have any errors values defined in RTE flow.
> > I don't want to enforce PMD with the error types.
> > If PMD can say that it can give better error code or add a case that may result in
> > error, I don't want to change the API.
> > So I think we better leave the error codes out of documentation unless they are final and can only
> > be resulted from the rte_level.
>
> It is not helpful for application. If so, application don't
> know how to interpret and handle various error codes.
Yes rte_flow error codes are not listed
(except for rte_flow_validate and indirect action).
As a consequence, the error code is mainly for debug purposes.
I am OK with being consistent and not listing error codes
in these new functions for now.
For consistency, I suggest removing error codes
from rte_flow_async_action_handle_* in this patchset.
We should have a general discussion about error codes handling later.
It may be a design decision to allow flexibility to PMDs.
If we want to provide some detailed error handling to applications,
we could list main or all kind of errors.
Anyway, this inconsistency is not new, so it should not block the patches IMHO.
@@ -3606,6 +3606,42 @@ 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 engine configuration and allocates
+requested resources beforehand to avoid costly allocations later.
+Expected number of resources in an application allows PMD to prepare
+and optimize NIC hardware configuration and 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 the number of available resources can be retrieved via
+``rte_flow_info_get()`` API.
+
+.. 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
@@ -99,6 +99,12 @@ New Features
The information of these properties is important for debug.
As the information is private, a dump function is introduced.
+* ** 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 available resources.
+
* **Updated AF_XDP PMD**
* Added support for libxdp >=v1.2.2.
@@ -138,7 +138,12 @@ struct rte_eth_dev_data {
* Indicates whether the device is configured:
* CONFIGURED(1) / NOT CONFIGURED(0)
*/
- dev_configured : 1;
+ dev_configured:1,
+ /**
+ * Indicates whether the flow engine is configured:
+ * CONFIGURED(1) / NOT CONFIGURED(0)
+ */
+ flow_configured:1;
/** Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0) */
uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
@@ -1392,3 +1392,72 @@ 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 (port_info == NULL) {
+ RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
+ return -EINVAL;
+ }
+ if (dev->data->dev_configured == 0) {
+ RTE_FLOW_LOG(INFO,
+ "Device with port_id=%"PRIu16" is not configured.\n",
+ port_id);
+ return -EINVAL;
+ }
+ if (unlikely(!ops))
+ return -rte_errno;
+ if (likely(!!ops->info_get)) {
+ return flow_err(port_id,
+ ops->info_get(dev, port_info, error),
+ 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);
+ int ret;
+
+ dev->data->flow_configured = 0;
+ if (port_attr == NULL) {
+ RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id);
+ return -EINVAL;
+ }
+ if (dev->data->dev_configured == 0) {
+ RTE_FLOW_LOG(INFO,
+ "Device with port_id=%"PRIu16" is not configured.\n",
+ port_id);
+ return -EINVAL;
+ }
+ if (dev->data->dev_started != 0) {
+ RTE_FLOW_LOG(INFO,
+ "Device with port_id=%"PRIu16" already started.\n",
+ port_id);
+ return -EINVAL;
+ }
+ if (unlikely(!ops))
+ return -rte_errno;
+ if (likely(!!ops->configure)) {
+ ret = ops->configure(dev, port_attr, error);
+ if (ret == 0)
+ dev->data->flow_configured = 1;
+ return flow_err(port_id, ret, error);
+ }
+ return rte_flow_error_set(error, ENOTSUP,
+ RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+ NULL, rte_strerror(ENOTSUP));
+}
@@ -43,6 +43,9 @@
extern "C" {
#endif
+#define RTE_FLOW_LOG(level, ...) \
+ rte_log(RTE_LOG_ ## level, rte_eth_dev_logtype, "" __VA_ARGS__)
+
/**
* Flow rule attributes.
*
@@ -4872,6 +4875,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 flow engine resources.
+ * The zero value means a resource is not supported.
+ *
+ */
+struct rte_flow_port_info {
+ /**
+ * Maximum number of counters.
+ * @see RTE_FLOW_ACTION_TYPE_COUNT
+ */
+ uint32_t max_nb_counters;
+ /**
+ * Maximum number of aging objects.
+ * @see RTE_FLOW_ACTION_TYPE_AGE
+ */
+ uint32_t max_nb_aging_objects;
+ /**
+ * Maximum number traffic meters.
+ * @see RTE_FLOW_ACTION_TYPE_METER
+ */
+ uint32_t max_nb_meters;
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Get information about flow engine 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 resources 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.
+ *
+ * Flow engine resources settings.
+ * The zero value means on demand resource allocations only.
+ *
+ */
+struct rte_flow_port_attr {
+ /**
+ * Number of counters to configure.
+ * @see RTE_FLOW_ACTION_TYPE_COUNT
+ */
+ uint32_t nb_counters;
+ /**
+ * Number of aging objects to configure.
+ * @see RTE_FLOW_ACTION_TYPE_AGE
+ */
+ uint32_t nb_aging_objects;
+ /**
+ * Number of traffic meters to configure.
+ * @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
@@ -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);
};
/**
@@ -264,6 +264,8 @@ EXPERIMENTAL {
rte_eth_ip_reassembly_capability_get;
rte_eth_ip_reassembly_conf_get;
rte_eth_ip_reassembly_conf_set;
+ rte_flow_info_get;
+ rte_flow_configure;
};
INTERNAL {