From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7F71FA034E; Mon, 21 Feb 2022 10:47:55 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0050B4068C; Mon, 21 Feb 2022 10:47:55 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 806174013F for ; Mon, 21 Feb 2022 10:47:54 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id C5C2D40; Mon, 21 Feb 2022 12:47:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru C5C2D40 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645436873; bh=sT1PqCorXVrNblcOYcDnDFDQfVTjRGvzCZTqWVuddUY=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Rf9Qu8Y4YEgGVSlOpC06m3xXEIb/B0bavwU9RqVl+kEjF2G3M1wme6XGX2V4oF6S2 GHDWTRV1gA7uYeVKeZVlkJ/OCdEnzmP1UHHueFeVvumJ5H8Uh32qOV+UJTHFHeftOE 5gS1VT8rafK+Vai7I8Uu4GvywhTQfFGN420IULxM= Message-ID: Date: Mon, 21 Feb 2022 12:47:53 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v8 01/11] ethdev: introduce flow engine configuration Content-Language: en-US To: Alexander Kozyrev , dev@dpdk.org Cc: orika@nvidia.com, thomas@monjalon.net, ivan.malov@oktetlabs.ru, ferruh.yigit@intel.com, mohammad.abdul.awal@intel.com, qi.z.zhang@intel.com, jerinj@marvell.com, ajit.khaparde@broadcom.com, bruce.richardson@intel.com References: <20220219041144.2145380-1-akozyrev@nvidia.com> <20220220034409.2226860-1-akozyrev@nvidia.com> <20220220034409.2226860-2-akozyrev@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220220034409.2226860-2-akozyrev@nvidia.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > Acked-by: Ori Kam [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]