Hi Andrew and Alexander,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@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 <akozy...@nvidia.com>
> >> Acked-by: Ori Kam <or...@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
