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 9DD99A0C55; Wed, 13 Oct 2021 13:26:12 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 23754410DA; Wed, 13 Oct 2021 13:26:12 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id AA74B40E64 for ; Wed, 13 Oct 2021 13:26:10 +0200 (CEST) Received: from [100.65.5.102] (unknown [5.144.123.99]) (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 1FB5B7F514; Wed, 13 Oct 2021 14:26:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 1FB5B7F514 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1634124369; bh=crOBjxs3eFfUqMQ5TTv2YNuxYTM2FrJv5Q7dIyL4LE4=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=iaSBhiJPtX6oPPpZmqHZ7dPmchbL72ICDywS2fhKMDJR/1/gtLWPu+KhUWqFiqzsI SiCvQ7fQE3OqmvCNX1AiYD8BQLpVF+eI7Fe9azaPv+nBE4G909D2ONfQN42qypwuY6 Pq1zsIC39QDlQdghdJxHa0dlnpP35H22xHm9WHaM= To: Alexander Kozyrev , "dev@dpdk.org" Cc: NBU-Contact-Thomas Monjalon , Ori Kam , "andrew.rybchenko@oktetlabs.ru" , "ferruh.yigit@intel.com" , "mohammad.abdul.awal@intel.com" , "qi.z.zhang@intel.com" , "jerinj@marvell.com" , "ajit.khaparde@broadcom.com" References: <20211006044835.3936226-1-akozyrev@nvidia.com> <20211006044835.3936226-3-akozyrev@nvidia.com> <5aecc54e-3a11-bbc6-af93-5117f938ee18@oktetlabs.ru> From: Ivan Malov Message-ID: <33876694-21e8-eeeb-3344-75854cf1d9e9@oktetlabs.ru> Date: Wed, 13 Oct 2021 14:25:57 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 2/3] ethdev: add flow item/action templates 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 Sender: "dev" Hi, On 13/10/2021 04:25, Alexander Kozyrev wrote: >> From: Ivan Malov On Wednesday, October 6, 2021 13:25 >> On 06/10/2021 07:48, Alexander Kozyrev wrote: >>> Treating every single flow rule as a completely independent and separate >>> entity negatively impacts the flow rules insertion rate. Oftentimes in an >>> application, many flow rules share a common structure (the same item >> mask >>> and/or action list) so they can be grouped and classified together. >>> This knowledge may be used as a source of optimization by a PMD/HW. >>> >>> The item template defines common matching fields (the item mask) >> without >>> values. The action template holds a list of action types that will be used >>> together in the same rule. The specific values for items and actions will >>> be given only during the rule creation. >>> >>> A table combines item and action templates along with shared flow rule >>> attributes (group ID, priority and traffic direction). This way a PMD/HW >>> can prepare all the resources needed for efficient flow rules creation in >>> the datapath. To avoid any hiccups due to memory reallocation, the >> maximum >>> number of flow rules is defined at table creation time. >>> >>> The flow rule creation is done by selecting a table, an item template >>> and an action template (which are bound to the table), and setting unique >>> values for the items and actions. >>> >>> Signed-off-by: Alexander Kozyrev >>> Suggested-by: Ori Kam >>> --- >>> lib/ethdev/rte_flow.h | 268 >> ++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 268 insertions(+) >>> >>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h >>> index c69d503b90..ba3204b17e 100644 >>> --- a/lib/ethdev/rte_flow.h >>> +++ b/lib/ethdev/rte_flow.h >>> @@ -4358,6 +4358,274 @@ int >>> rte_flow_configure(uint16_t port_id, >>> const struct rte_flow_port_attr *port_attr, >>> struct rte_flow_error *error); >>> + >>> +/** >>> + * Opaque type returned after successfull creation of item template. >> >> Typo: "successfull" --> "successful". > Thanks for noticing, will correct. > >>> + * This handle can be used to manage the created item template. >>> + */ >>> +struct rte_flow_item_template; >>> + >>> +__extension__ >>> +struct rte_flow_item_template_attr { >>> + /** >>> + * Version of the struct layout, should be 0. >>> + */ >>> + uint32_t version; >>> + /* No attributes so far. */ >>> +}; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Create item template. >>> + * The item template defines common matching fields (item mask) >> without values. >>> + * For example, matching on 5 tuple TCP flow, the template will be >>> + * eth(null) + IPv4(source + dest) + TCP(s_port + d_port), >>> + * while values for each rule will be set during the flow rule creation. >>> + * The order of items in the template must be the same at rule insertion. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] attr >>> + * Item template attributes. >> >> Please consider adding meaningful prefixes to "attr" here and below. >> This is needed to avoid confusion with "struct rte_flow_attr". >> >> Example: "template_attr". > No problem. > >>> + * @param[in] items >>> + * Pattern specification (list terminated by the END pattern item). >>> + * The spec member of an item is not used unless the end member is >> used. >>> + * @param[out] error >>> + * Perform verbose error reporting if not NULL. >>> + * PMDs initialize this structure in case of error only. >>> + * >>> + * @return >>> + * Handle on success, NULL otherwise and rte_errno is set. >>> + */ >>> +__rte_experimental >>> +struct rte_flow_item_template * >>> +rte_flow_item_template_create(uint16_t port_id, >>> + const struct rte_flow_item_template_attr *attr, >>> + const struct rte_flow_item items[], >>> + struct rte_flow_error *error); >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Destroy item template. >>> + * This function may be called only when >>> + * there are no more tables referencing this template. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] template >>> + * Handle to the template to be destroyed. >> >> Perhaps "handle OF the template"? > You are right. > >>> + * @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_item_template_destroy(uint16_t port_id, >>> + struct rte_flow_item_template *template, >>> + struct rte_flow_error *error); >>> + >>> +/** >>> + * Opaque type returned after successfull creation of action template. >> >> Single "l" in "successful". > Ditto. > >>> + * This handle can be used to manage the created action template. >>> + */ >>> +struct rte_flow_action_template; >>> + >>> +__extension__ >>> +struct rte_flow_action_template_attr { >>> + /** >>> + * Version of the struct layout, should be 0. >>> + */ >>> + uint32_t version; >>> + /* No attributes so far. */ >>> +}; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Create action template. >>> + * The action template holds a list of action types without values. >>> + * For example, the template to change TCP ports is TCP(s_port + d_port), >>> + * while values for each rule will be set during the flow rule creation. >>> + * >>> + * The order of the action in the template must be kept when inserting >> rules. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] attr >>> + * Template attributes. >> >> Perhaps add a meaningful prefix to "attr". > Sure thing, will rename all the "attr" to "thing_attr". > >>> + * @param[in] actions >>> + * Associated actions (list terminated by the END action). >>> + * The spec member is only used if the mask is 1. >> >> Maybe "its mask is all ones"? > Not necessarily, just non-zero value would do. Will make it clearer. > >>> + * @param[in] masks >>> + * List of actions that marks which of the action's member is constant. >> >> Consider the following action example: >> >> struct rte_flow_action_vxlan_encap { >> struct rte_flow_item *definition; >> }; >> >> So, if "definition" is not NULL, the whole header definition is supposed >> to be constant, right? Or am I missing something? > If definition has non-zero value then the action spec will be used in every rule created with this template. > In this particular example, yes, this definition is going to be a constant header for all the rules. > > >>> + * A mask has the same format as the corresponding action. >>> + * If the action field in @p masks is not 0, >>> + * the corresponding value in an action from @p actions will be the part >>> + * of the template and used in all flow rules. >>> + * The order of actions in @p masks is the same as in @p actions. >>> + * In case of indirect actions present in @p actions, >>> + * the actual action type should be present in @p mask. >>> + * @param[out] error >>> + * Perform verbose error reporting if not NULL. >>> + * PMDs initialize this structure in case of error only. >>> + * >>> + * @return >>> + * handle on success, NULL otherwise and rte_errno is set. >>> + */ >>> +__rte_experimental >>> +struct rte_flow_action_template * >>> +rte_flow_action_template_create(uint16_t port_id, >>> + const struct rte_flow_action_template_attr *attr, >>> + const struct rte_flow_action actions[], >>> + const struct rte_flow_action masks[], >>> + struct rte_flow_error *error); >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Destroy action template. >>> + * This function may be called only when >>> + * there are no more tables referencing this template. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] template >>> + * Handle to the template to be destroyed. >>> + * @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_action_template_destroy(uint16_t port_id, >>> + const struct rte_flow_action_template *template, >>> + struct rte_flow_error *error); >>> + >>> + >>> +/** >>> + * Opaque type returned after successfull creation of table. >> >> Redundant "l" in "successful". > Consider this fixed. > >>> + * This handle can be used to manage the created table. >>> + */ >>> +struct rte_flow_table; >>> + >>> +enum rte_flow_table_mode { >>> + /** >>> + * Fixed size, the number of flow rules will be limited. >>> + * It is possible that some of the rules will not be inserted >>> + * due to conflicts/lack of space. >>> + * When rule insertion fails with try again error, >>> + * the application may use one of the following ways >>> + * to address this state: >>> + * 1. Keep this rule processing in the software. >>> + * 2. Try to offload this rule at a later time, >>> + * after some rules have been removed from the hardware. >>> + * 3. Create a new table and add this rule to the new table. >>> + */ >>> + RTE_FLOW_TABLE_MODE_FIXED, >>> + /** >>> + * Resizable, the PMD/HW will insert all rules. >>> + * No try again error will be received in this mode. >>> + */ >>> + RTE_FLOW_TABLE_MODE_RESIZABLE, >>> +}; >>> + >>> +/** >>> + * Table attributes. >>> + */ >>> +struct rte_flow_table_attr { >>> + /** >>> + * Version of the struct layout, should be 0. >>> + */ >>> + uint32_t version; >>> + /** >>> + * Flow attributes that will be used in the table. >>> + */ >>> + struct rte_flow_attr attr; >> >> Perhaps, "flow_attr" then? > As we agreed. > >>> + /** >>> + * Maximum number of flow rules that this table holds. >>> + * It can be hard or soft limit depending on the mode. >>> + */ >>> + uint32_t max_rules; >> >> How about "nb_flows_max"? > Just nb_flows maybe? > Probabably OK, too, but I don't have a strong opinion. One more option: "table_size". -- Ivan M