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 4C4BBA00C5; Wed, 16 Feb 2022 14:03:24 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CEE24410FB; Wed, 16 Feb 2022 14:03:23 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A4D0240150 for ; Wed, 16 Feb 2022 14:03:22 +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 C8F2644; Wed, 16 Feb 2022 16:03:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru C8F2644 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645016602; bh=V2WQuml9Lp85aelUZwpjLtkbWlnFl7tw9ouMPIyMhXo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=PBN3JmBAZNb9BuZ93T26Yv2dQmmeYjObToZJ8DeoaqjqEsyGnsH8V52SrIdPRh1Wx O2bFjQPbrLWdvmXosyDcZB77ny9xYLb0VZ/3jeFgc5zDLpwvzr6Pm1fnS/lJ8lRcmd gI9AA3i/97sQBDpeyoRCG9Y26cFrtaVA31GBRWvA= Message-ID: <982e92be-5e58-7e63-4bf4-7f095e45fcbd@oktetlabs.ru> Date: Wed, 16 Feb 2022 16:03:21 +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 v5 01/10] ethdev: introduce flow pre-configuration hints Content-Language: en-US To: Alexander Kozyrev , "dev@dpdk.org" Cc: Ori Kam , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , "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: <20220209213809.1208269-1-akozyrev@nvidia.com> <20220211022653.1372318-1-akozyrev@nvidia.com> <20220211022653.1372318-2-akozyrev@nvidia.com> <920abf38-3cb6-cea2-7667-f785becd0751@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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/11/22 21:47, Alexander Kozyrev wrote: > On Friday, February 11, 2022 5:17 Andrew Rybchenko wrote: >> Sent: Friday, February 11, 2022 5:17 >> To: Alexander Kozyrev ; dev@dpdk.org >> Cc: Ori Kam ; NBU-Contact-Thomas Monjalon (EXTERNAL) >> ; 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 >> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints >> >> On 2/11/22 05:26, 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/rte_flow.c b/lib/ethdev/rte_flow.c >>> index a93f68abbc..66614ae29b 100644 >>> --- a/lib/ethdev/rte_flow.c >>> +++ b/lib/ethdev/rte_flow.c >>> @@ -1391,3 +1391,43 @@ 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 (unlikely(!ops)) >>> + return -rte_errno; >>> + if (likely(!!ops->info_get)) { >> >> expected ethdev state must be validated. Just configured? >> >>> + return flow_err(port_id, >>> + ops->info_get(dev, port_info, error), >> >> port_info must be checked vs NULL > > We don’t have any NULL checks for parameters in the whole ret flow API library. > See rte_flow_create() for example. attributes, pattern and actions are passed to PMD unchecked. IMHO it is hardly a good reason to have no such check here. The API is pure control path. So, it must validate all input arguments and it is better to do it in a generic place. >>> + 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); >>> + >>> + if (unlikely(!ops)) >>> + return -rte_errno; >>> + if (likely(!!ops->configure)) { >> >> The API must validate ethdev state. configured and not started? > Again, we have no such validation for any rte flow API today. Same here. If documentation defines in which state the API should be called, generic code must guarantee it. >> >>> + return flow_err(port_id, >>> + ops->configure(dev, port_attr, error), >> >> port_attr must be checked vs NULL > Same. > >>> + error); >>> + } >>> + return rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> +} >>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h >>> index 1031fb246b..92be2a9a89 100644 >>> --- a/lib/ethdev/rte_flow.h >>> +++ b/lib/ethdev/rte_flow.h >>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t port_id, >>> const struct rte_flow_item_flex_handle *handle, >>> struct rte_flow_error *error); >>> >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Information about available pre-configurable resources. >>> + * The zero value means a resource cannot be pre-allocated. >>> + * >>> + */ >>> +struct rte_flow_port_info { >>> + /** >>> + * Number of pre-configurable counter actions. >>> + * @see RTE_FLOW_ACTION_TYPE_COUNT >>> + */ >>> + uint32_t nb_counters; >> >> Name says that it is a number of counters, but description >> says that it is about actions. >> Also I don't understand what does "pre-configurable" mean. >> Isn't it a maximum number of available counters? >> If no, how can I find a maximum? > It is number of pre-allocated and pre-configured actions. > How are they pr-configured is up to PDM driver. > But let's change to "pre-configured" everywhere. > Configuration includes some memory allocation anyway. Sorry, but I still don't understand. I guess HW has a hard limit on a number of counters. How can I get the information? >> >>> + /** >>> + * Number of pre-configurable aging flows actions. >>> + * @see RTE_FLOW_ACTION_TYPE_AGE >>> + */ >>> + uint32_t nb_aging_flows; >> >> Same > Ditto. > >>> + /** >>> + * Number of pre-configurable traffic metering actions. >>> + * @see RTE_FLOW_ACTION_TYPE_METER >>> + */ >>> + uint32_t nb_meters; >> >> Same > Ditto. > >>> +}; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Retrieve configuration attributes supported by the port. >> >> Description should be a bit more flow API aware. >> Right now it sounds too generic. > Ok, how about > "Get information about flow engine pre-configurable 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 contextual 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. >>> + */ >>> +__rte_experimental >>> +int >>> +rte_flow_info_get(uint16_t port_id, >>> + struct rte_flow_port_info *port_info, >>> + struct rte_flow_error *error); >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Resource pre-allocation and pre-configuration settings. >> >> What is the difference between pre-allocation and pre-configuration? >> Why are both mentioned above, but just pre-configured actions are >> mentioned below? > Please see answer to this question above. > >>> + * The zero value means on demand resource allocations only. >>> + * >>> + */ >>> +struct rte_flow_port_attr { >>> + /** >>> + * Number of counter actions pre-configured. >>> + * @see RTE_FLOW_ACTION_TYPE_COUNT >>> + */ >>> + uint32_t nb_counters; >>> + /** >>> + * Number of aging flows actions pre-configured. >>> + * @see RTE_FLOW_ACTION_TYPE_AGE >>> + */ >>> + uint32_t nb_aging_flows; >>> + /** >>> + * Number of traffic metering actions pre-configured. >>> + * @see RTE_FLOW_ACTION_TYPE_METER >>> + */ >>> + uint32_t nb_meters; >>> +}; >>> + >>> +/** >>> + * @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. >>> + */ >>> +__rte_experimental >>> +int >>> +rte_flow_configure(uint16_t port_id, >>> + const struct rte_flow_port_attr *port_attr, >>> + struct rte_flow_error *error); >>> + >>> #ifdef __cplusplus >>> } >>> #endif [snip]