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

[Andrew Rybchenko]

On 2/21/22 15:53, Ori Kam wrote:
> Hi Andrew and Alexander,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko at 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 at nvidia.com>
>>>> Acked-by: Ori Kam <orika at 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