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 C6CD3A034E; Mon, 21 Feb 2022 11:57:31 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B45CC4068C; Mon, 21 Feb 2022 11:57:31 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E32B54013F for ; Mon, 21 Feb 2022 11:57:29 +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 31F4840; Mon, 21 Feb 2022 13:57:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 31F4840 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645441049; bh=/HtId2da8kKlegayaBR2q1rC2a1z+Q5kcUhsugtKAPE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=s4E/vdCdE10bH4cNnTnoTOwgpAgWzFG8/2U3zO5DrohCEVfJpm9szm1FVfgw2H+JA 1rRFTCW0OjEN/CAdlycg914GnvVdjfOFJ2MWtS4C1wx7fgUVf4H4GCUL1+6YWZSvWM R9E/10v6WvLRNntJFkrFaq8ohQ485c9I1q95H5A4= Message-ID: Date: Mon, 21 Feb 2022 13:57:28 +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: 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-3-akozyrev@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220220034409.2226860-3-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: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. > + [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. > + */ > +__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. > + /** 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. [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. > + * 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]