Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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]