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.
+ /**
+ * 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]
