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 DFA66A034E; Mon, 21 Feb 2022 16:05:46 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6E4EA40E5A; Mon, 21 Feb 2022 16:05:46 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 19E0040DF6 for ; Mon, 21 Feb 2022 16:05:45 +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 3E83638; Mon, 21 Feb 2022 18:05:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3E83638 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645455944; bh=IVZb8GNhHw+8ij3nodrq1zjBgzDS+IstLz51NunBcZk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=IALGOaJeFCZ+Wgpc6zGNF6joDAbe29Qtgrpk3//1F6eGCJoqysbLAYCNebXOrtdCC vAiP+wIP5at8bHOZgpkq0VPUdv9eO7kPTb8J2s+kKCoPXmh/L6ZxkRzqI6JylyrV1T VlsD/Ry4DWrRuUfLgI39SB2ieKnAD6wNk47wrvCY= Message-ID: <5d8e0b32-f674-a43f-20ee-a689ab7c2012@oktetlabs.ru> Date: Mon, 21 Feb 2022 18:05:43 +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 02/11] ethdev: add flow item/action templates Content-Language: en-US To: Ori Kam , Alexander Kozyrev , "dev@dpdk.org" Cc: "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: <20220219041144.2145380-1-akozyrev@nvidia.com> <20220220034409.2226860-1-akozyrev@nvidia.com> <20220220034409.2226860-3-akozyrev@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: 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/21/22 16:12, Ori Kam wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, February 21, 2022 12:57 PM >> Subject: Re: [PATCH v8 02/11] ethdev: add flow item/action templates >> >> On 2/20/22 06:44, 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 pattern template defines common matching fields (the item mask) without >>> values. The actions 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 pattern and actions 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 the table creation time. >>> >>> The flow rule creation is done by selecting a table, a pattern template >>> and an actions template (which are bound to the table), and setting unique >>> values for the items and actions. >>> >>> Signed-off-by: Alexander Kozyrev >>> Acked-by: Ori Kam >> >> [snip] >> >>> +For example, to create an actions template with the same Mark ID >>> +but different Queue Index for every rule: >>> + >>> +.. code-block:: c >>> + >>> + rte_flow_actions_template_attr attr = {.ingress = 1}; >>> + struct rte_flow_action act[] = { >>> + /* Mark ID is 4 for every rule, Queue Index is unique */ >>> + [0] = {.type = RTE_FLOW_ACTION_TYPE_MARK, >>> + .conf = &(struct rte_flow_action_mark){.id = 4}}, >>> + [1] = {.type = RTE_FLOW_ACTION_TYPE_QUEUE}, >>> + [2] = {.type = RTE_FLOW_ACTION_TYPE_END,}, >>> + }; >>> + struct rte_flow_action msk[] = { >>> + /* Assign to MARK mask any non-zero value to make it constant */ >>> + [0] = {.type = RTE_FLOW_ACTION_TYPE_MARK, >>> + .conf = &(struct rte_flow_action_mark){.id = 1}}, >> >> 1 looks very strange. I can understand it in the case of >> integer and boolean fields, but what to do in the case of >> arrays? IMHO, it would be better to use all 0xff's in value. >> Anyway, it must be defined very carefully and non-ambiguous. >> > There is some issues with all 0xff for example in case of pointers or > enums this it will result in invalid value. > So I vote for saving it as is. > I fully agree that it should be defined very clearly. > I think that for arrays with predefined size (I don't think we have such in rte_flow) > it should be declared that that the first element should not be 0. It is good that we agree that the aspect should be documented very carefully. Let's do it. > >>> + [1] = {.type = RTE_FLOW_ACTION_TYPE_QUEUE}, >>> + [2] = {.type = RTE_FLOW_ACTION_TYPE_END,}, >>> + }; >>> + struct rte_flow_error err; >>> + >>> + struct rte_flow_actions_template *actions_template = >>> + rte_flow_actions_template_create(port, &attr, &act, &msk, &err); >>> + >>> +The concrete value for Queue Index will be provided at the rule creation. >> >> [snip] >> >>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c >>> index ffd48e40d5..e9f684eedb 100644 >>> --- a/lib/ethdev/rte_flow.c >>> +++ b/lib/ethdev/rte_flow.c >>> @@ -1461,3 +1461,255 @@ rte_flow_configure(uint16_t port_id, >>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> NULL, rte_strerror(ENOTSUP)); >>> } >>> + >>> +struct rte_flow_pattern_template * >>> +rte_flow_pattern_template_create(uint16_t port_id, >>> + const struct rte_flow_pattern_template_attr *template_attr, >>> + const struct rte_flow_item pattern[], >>> + 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); >>> + struct rte_flow_pattern_template *template; >>> + >>> + if (template_attr == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" template attr is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (pattern == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" pattern is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (dev->data->flow_configured == 0) { >>> + RTE_FLOW_LOG(INFO, >>> + "Flow engine on port_id=%"PRIu16" is not configured.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_STATE, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (unlikely(!ops)) >>> + return NULL; >> >> See notes about order of checks in previous patch review notes. >> >>> + if (likely(!!ops->pattern_template_create)) { >>> + template = ops->pattern_template_create(dev, template_attr, >>> + pattern, error); >>> + if (template == NULL) >>> + flow_err(port_id, -rte_errno, error); >>> + return template; >>> + } >>> + rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> + return NULL; >>> +} >>> + >>> +int >>> +rte_flow_pattern_template_destroy(uint16_t port_id, >>> + struct rte_flow_pattern_template *pattern_template, >>> + 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(pattern_template == NULL)) >>> + return 0; >>> + if (unlikely(!ops)) >>> + return -rte_errno; >> >> Same here. I'm afraid it is really important here as well, >> since request should not return OK if port_id is invalid. >> >> >>> + if (likely(!!ops->pattern_template_destroy)) { >>> + return flow_err(port_id, >>> + ops->pattern_template_destroy(dev, >>> + pattern_template, >>> + error), >>> + error); >>> + } >>> + return rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> +} >>> + >>> +struct rte_flow_actions_template * >>> +rte_flow_actions_template_create(uint16_t port_id, >>> + const struct rte_flow_actions_template_attr *template_attr, >>> + const struct rte_flow_action actions[], >>> + const struct rte_flow_action masks[], >>> + 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); >>> + struct rte_flow_actions_template *template; >>> + >>> + if (template_attr == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" template attr is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (actions == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" actions is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (masks == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" masks is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + >>> + } >>> + if (dev->data->flow_configured == 0) { >>> + RTE_FLOW_LOG(INFO, >>> + "Flow engine on port_id=%"PRIu16" is not configured.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_STATE, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (unlikely(!ops)) >>> + return NULL; >> >> same here >> >>> + if (likely(!!ops->actions_template_create)) { >>> + template = ops->actions_template_create(dev, template_attr, >>> + actions, masks, error); >>> + if (template == NULL) >>> + flow_err(port_id, -rte_errno, error); >>> + return template; >>> + } >>> + rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> + return NULL; >>> +} >>> + >>> +int >>> +rte_flow_actions_template_destroy(uint16_t port_id, >>> + struct rte_flow_actions_template *actions_template, >>> + 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(actions_template == NULL)) >>> + return 0; >>> + if (unlikely(!ops)) >>> + return -rte_errno; >> >> same here >> >>> + if (likely(!!ops->actions_template_destroy)) { >>> + return flow_err(port_id, >>> + ops->actions_template_destroy(dev, >>> + actions_template, >>> + error), >>> + error); >>> + } >>> + return rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> +} >>> + >>> +struct rte_flow_template_table * >>> +rte_flow_template_table_create(uint16_t port_id, >>> + const struct rte_flow_template_table_attr *table_attr, >>> + struct rte_flow_pattern_template *pattern_templates[], >>> + uint8_t nb_pattern_templates, >>> + struct rte_flow_actions_template *actions_templates[], >>> + uint8_t nb_actions_templates, >>> + 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); >>> + struct rte_flow_template_table *table; >>> + >>> + if (table_attr == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" table attr is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (pattern_templates == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" pattern templates is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (actions_templates == NULL) { >>> + RTE_FLOW_LOG(ERR, >>> + "Port %"PRIu16" actions templates is NULL.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_ATTR, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (dev->data->flow_configured == 0) { >>> + RTE_FLOW_LOG(INFO, >>> + "Flow engine on port_id=%"PRIu16" is not configured.\n", >>> + port_id); >>> + rte_flow_error_set(error, EINVAL, >>> + RTE_FLOW_ERROR_TYPE_STATE, >>> + NULL, rte_strerror(EINVAL)); >>> + return NULL; >>> + } >>> + if (unlikely(!ops)) >>> + return NULL; >> >> Order of checks >> >>> + if (likely(!!ops->template_table_create)) { >>> + table = ops->template_table_create(dev, table_attr, >>> + pattern_templates, nb_pattern_templates, >>> + actions_templates, nb_actions_templates, >>> + error); >>> + if (table == NULL) >>> + flow_err(port_id, -rte_errno, error); >>> + return table; >>> + } >>> + rte_flow_error_set(error, ENOTSUP, >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>> + NULL, rte_strerror(ENOTSUP)); >>> + return NULL; >>> +} >>> + >>> +int >>> +rte_flow_template_table_destroy(uint16_t port_id, >>> + struct rte_flow_template_table *template_table, >>> + 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(template_table == NULL)) >>> + return 0; >>> + if (unlikely(!ops)) >>> + return -rte_errno; >>> + if (likely(!!ops->template_table_destroy)) { >>> + return flow_err(port_id, >>> + ops->template_table_destroy(dev, >>> + template_table, >>> + error), >>> + 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 cdb7b2be68..776e8ccc11 100644 >>> --- a/lib/ethdev/rte_flow.h >>> +++ b/lib/ethdev/rte_flow.h >>> @@ -4983,6 +4983,280 @@ rte_flow_configure(uint16_t port_id, >>> const struct rte_flow_port_attr *port_attr, >>> struct rte_flow_error *error); >>> >>> +/** >>> + * Opaque type returned after successful creation of pattern template. >>> + * This handle can be used to manage the created pattern template. >>> + */ >>> +struct rte_flow_pattern_template; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Flow pattern template attributes. >> >> Would it be useful to mentioned that at least one direction >> bit must be set? Otherwise request does not make sense. >> > Agree one direction must be set. > >>> + */ >>> +__extension__ >>> +struct rte_flow_pattern_template_attr { >>> + /** >>> + * Relaxed matching policy. >>> + * - PMD may match only on items with mask member set and skip >>> + * matching on protocol layers specified without any masks. >>> + * - If not set, PMD will match on protocol layers >>> + * specified without any masks as well. >>> + * - Packet data must be stacked in the same order as the >>> + * protocol layers to match inside packets, starting from the lowest. >>> + */ >>> + uint32_t relaxed_matching:1; >> >> I should notice this earlier, but it looks like a new feature >> which sounds unrelated to templates. If so, it makes asymmetry >> in sync and async flow rules capabilities. >> Am I missing something? >> >> Anyway, the feature looks hidden in the patch. >> > No this is not hidden feature. > In current API application must specify all the preciding items, > For example application wants to match on udp source port. > The rte flow will look something like eth / ipv4/ udp sport = xxx .. > When PMD gets this pattern it must enforce the after the eth > there will be IPv4 and then UDP and then add the match for the > sport. > This means that the PMD addes extra matching. > If the application already validated that there is udp in the packet > in group 0 and then jump to group 1 it can save the HW those extra matching > by enabling this bit which means that the HW should only match on implicit > masked fields. Old API allows to insert rule to non-0 table as well. So, similar logic could be applicable. Do we want to have the same feature in old API? > >>> + /** Pattern valid for rules applied to ingress traffic. */ >>> + uint32_t ingress:1; >>> + /** Pattern valid for rules applied to egress traffic. */ >>> + uint32_t egress:1; >>> + /** Pattern valid for rules applied to transfer traffic. */ >>> + uint32_t transfer:1; >>> +}; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Create flow pattern template. >>> + * >>> + * The pattern template defines common matching fields 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 number and order of items in the template must be the same >>> + * at the rule creation. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] template_attr >>> + * Pattern template attributes. >>> + * @param[in] pattern >>> + * 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. >> >> Don't we want to be explicit about used negative error code? >> The question is applicable to all functions. >> > Same answer as given in other patch. > Since PMD may have different/extra error codes I don't think we should > give them here. > >> [snip] >> >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Flow actions template attributes. >> >> Same question about no directions specified. >> >>> + */ >>> +__extension__ >>> +struct rte_flow_actions_template_attr { >>> + /** Action valid for rules applied to ingress traffic. */ >>> + uint32_t ingress:1; >>> + /** Action valid for rules applied to egress traffic. */ >>> + uint32_t egress:1; >>> + /** Action valid for rules applied to transfer traffic. */ >>> + uint32_t transfer:1; >>> +}; >>> + >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change without prior notice. >>> + * >>> + * Create flow actions template. >>> + * >>> + * The actions 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 number and order of actions in the template must be the same >>> + * at the rule creation. >>> + * >>> + * @param port_id >>> + * Port identifier of Ethernet device. >>> + * @param[in] template_attr >>> + * Template attributes. >>> + * @param[in] actions >>> + * Associated actions (list terminated by the END action). >>> + * The spec member is only used if @p masks spec is non-zero. >>> + * @param[in] masks >>> + * List of actions that marks which of the action's member is constant. >>> + * A mask has the same format as the corresponding action. >>> + * If the action field in @p masks is not 0, >> >> Comparison with zero makes sense for integers only. >> > > Why? It can also be with pointers enums. It should be NULL for pointers and enum-specific member of enum. > >>> + * 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_actions_template * >>> +rte_flow_actions_template_create(uint16_t port_id, >>> + const struct rte_flow_actions_template_attr *template_attr, >>> + const struct rte_flow_action actions[], >>> + const struct rte_flow_action masks[], >>> + struct rte_flow_error *error); >> >> [snip] > > Best, > Ori >