From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BE0B3A04B6; Mon, 12 Oct 2020 16:19:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3F5101D8FD; Mon, 12 Oct 2020 16:19:28 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by dpdk.org (Postfix) with ESMTP id 647AA1D8CA for ; Mon, 12 Oct 2020 16:19:27 +0200 (CEST) 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 042507F4F1; Mon, 12 Oct 2020 17:19:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 042507F4F1 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1602512366; bh=hk8ZGx3JwO6ZFqSb2GsLCxvOM1pC10VMghjq+3Z/4KU=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=YK0asV+i2iPA/pwda0wpwKJpWLKc5He7WbxSEDe/XIbcBqe97Kpjbfo13G3V0HNXq IJ8/saPswWQhjDdCw268+8oNcYAeNGHFofH/jy+R6syUpRLikJi0kJ6F5E1/k+QJTm bVAJaWH3JJ9OLjKFWTNAI16tEaXRUAnSHEIWiTkw= To: Andrey Vesnovaty , dev@dpdk.org Cc: jer@marvell.com, jerinjacobk@gmail.com, thomas@monjalon.net, ferruh.yigit@intel.com, stephen@networkplumber.org, bruce.richardson@intel.com, orika@nvidia.com, viacheslavo@nvidia.com, andrey.vesnovaty@gmail.com, mdr@ashroe.eu, nhorman@tuxdriver.com, ajit.khaparde@broadcom.com, samik.gupta@broadcom.com References: <20200702120511.16315-1-andreyv@mellanox.com> <20201008115143.13208-1-andreyv@nvidia.com> <20201008115143.13208-2-andreyv@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <3d2a7271-3e5b-9527-481b-5086e9a7affd@oktetlabs.ru> Date: Mon, 12 Oct 2020 17:19:25 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <20201008115143.13208-2-andreyv@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" "add flow shared action API" Is flow shared? May be "add shared actions to flow API". On 10/8/20 2:51 PM, Andrey Vesnovaty wrote: > This commit introduces extension of DPDK flow action API enabling "This commit" and "DPDK" are not necessary in description. It is a commit description and the patch is to DPDK tree. Consider just: "Introduce extension of flow action API enabling..." > sharing of single rte_flow_action in multiple flows. The API intended for > PMDs, where multiple HW offloaded flows can reuse the same HW > essence/object representing flow action and modification of such an > essence/object affects all the rules using it. > > Motivation and example > === > Adding or removing one or more queues to RSS used by multiple flow rules > imposes per rule toll for current DPDK flow API; the scenario requires > for each flow sharing cloned RSS action: > - call `rte_flow_destroy()` > - call `rte_flow_create()` with modified RSS action > > API for sharing action and its in-place update benefits: > - reduce the overhead of multiple RSS flow rules reconfiguration > - optimize resource utilization by sharing action across multiple > flows > > Change description > === > > Shared action > === > In order to represent flow action shared by multiple flows new action > type RTE_FLOW_ACTION_TYPE_SHARED is introduced (see `enum > rte_flow_action_type`). > Actually the introduced API decouples action from any specific flow and > enables sharing of single action by its handle across multiple flows. > > Shared action create/use/destroy > === > Shared action may be reused by some or none flow rules at any given > moment, i.e. shared action resides outside of the context of any flow. > Shared action represent HW resources/objects used for action offloading > implementation. > API for shared action create (see `rte_flow_shared_action_create()`): > - should allocate HW resources and make related initializations required > for shared action implementation. > - make necessary preparations to maintain shared access to > the action resources, configuration and state. > API for shared action destroy (see `rte_flow_shared_action_destroy()`) > should release HW resources and make related cleanups required for shared > action implementation. > > In order to share some flow action reuse the handle of type > `struct rte_flow_shared_action` returned by > rte_flow_shared_action_create() as a `conf` field of > `struct rte_flow_action` (see "example" section). > > If some shared action not used by any flow rule all resources allocated > by the shared action can be released by rte_flow_shared_action_destroy() > (see "example" section). The shared action handle passed as argument to > destroy API should not be used any further i.e. result of the usage is > undefined. > > Shared action re-configuration > === > Shared action behavior defined by its configuration can be updated via > rte_flow_shared_action_update() (see "example" section). The shared > action update operation modifies HW related resources/objects allocated > on the action creation. The number of operations performed by the update > operation should not depend on the number of flows sharing the related > action. On return of shared action update API action behavior should be > according to updated configuration for all flows sharing the action. > > Shared action query > === > Provide separate API to query shared action state (see > rte_flow_shared_action_update()). Taking a counter as an example: query > returns value aggregating all counter increments across all flow rules > sharing the counter. This API doesn't query shared action configuration > since it is controlled by rte_flow_shared_action_create() and > rte_flow_shared_action_update() APIs and no supposed to change by other > means. > > PMD support > === > The support of introduced API is pure PMD specific design and > responsibility for each action type (see struct rte_flow_ops). > > testpmd > === > In order to utilize introduced API testpmd cli may implement following > extension > create/update/destroy/query shared action accordingly > > flow shared_action (port) create {action_id (id)} (action) / end > flow shared_action (port) update (id) (action) / end > flow shared_action (port) destroy action_id (id) {action_id (id) [...]} > flow shared_action (port) query (id) > > testpmd example > === > > configure rss to queues 1 & 2 > >> flow shared_action 0 create action_id 100 rss queues 1 2 end / end > > create flow rule utilizing shared action > >> flow create 0 ingress \ > pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \ > actions shared 100 / end > > add 2 more queues > >> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end testpmd is out-of-scope of the patch and it is better to remove the description to avoid unsync if testpmd commands are changed. > > example > === > > struct rte_flow_action actions[2]; > struct rte_flow_shared_action_conf conf; > struct rte_flow_action action; > /* skipped: initialize conf and action */ > struct rte_flow_shared_action *handle = > rte_flow_shared_action_create(port_id, &conf, &action, &error); > actions[0].type = RTE_FLOW_ACTION_TYPE_SHARED; > actions[0].conf = handle; > actions[1].type = RTE_FLOW_ACTION_TYPE_END; > /* skipped: init attr0 & pattern0 args */ > struct rte_flow *flow0 = rte_flow_create(port_id, &attr0, pattern0, > actions, error); > /* create more rules reusing shared action */ > struct rte_flow *flow1 = rte_flow_create(port_id, &attr1, pattern1, > actions, error); > /* skipped: for flows 2 till N */ > struct rte_flow *flowN = rte_flow_create(port_id, &attrN, patternN, > actions, error); > /* update shared action */ > struct rte_flow_action updated_action; > /* > * skipped: initialize updated_action according to desired action > * configuration change > */ > rte_flow_shared_action_update(port_id, handle, &updated_action, error); > /* > * from now on all flows 1 till N will act according to configuration of > * updated_action > */ > /* skipped: destroy all flows 1 till N */ > rte_flow_shared_action_destroy(port_id, handle, error); > > Signed-off-by: Andrey Vesnovaty > Acked-by: Ori Kam LGTM > --- > doc/guides/prog_guide/rte_flow.rst | 19 +++ > doc/guides/rel_notes/release_20_11.rst | 9 ++ > lib/librte_ethdev/rte_ethdev_version.map | 4 + > lib/librte_ethdev/rte_flow.c | 84 +++++++++++ > lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++- > lib/librte_ethdev/rte_flow_driver.h | 23 +++ > 6 files changed, 307 insertions(+), 1 deletion(-) > > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index 119b128739..8cff8a0440 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -2666,6 +2666,25 @@ timeout passed without any matching on the flow. > | ``context`` | user input flow context | > +--------------+---------------------------------+ > > +Action: ``SHARED`` > +^^^^^^^^^^^^^^^^^^ > + > +Flow Utilize shared action by handle as returned from Utilize -> utilize > +``rte_flow_shared_action_create()``. > + > +The behaviour of the shared action defined by ``action`` argument of type > +``struct rte_flow_action`` passed to ``rte_flow_shared_action_create()``. > + > +.. _table_rte_flow_shared_action: > + > +.. table:: SHARED > + > + +---------------+ > + | Field | > + +===============+ > + | no properties | > + +---------------+ > + > Negative types > ~~~~~~~~~~~~~~ > > diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst > index 0b2a3700c3..87c90909be 100644 > --- a/doc/guides/rel_notes/release_20_11.rst > +++ b/doc/guides/rel_notes/release_20_11.rst > @@ -109,6 +109,15 @@ New Features > * Extern objects and functions can be plugged into the pipeline. > * Transaction-oriented table updates. > > +* **Add shared action support for rte flow.** I think it required "ethdev: " prefix. Add -> Added, rte -> RTE, plus API, i.e.: **ethdev: Added shared action support to RTE flow API." > + > + Added shared action support to utilize single rte flow action in multiple rte -> RTE, but I'd consider to drop RTE in both description, here and below. > + rte flow rules. An update of shared action configuration alters the behavior > + of all rte flow rules using it. > + > + * Added new action: ``RTE_FLOW_ACTION_TYPE_SHARED`` to use shared action > + as rte flow action. > + * Added new rte flow APIs to create/update/destroy/query shared action. Missing one more empty line here. > > Removed Items > ------------- > diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map > index c95ef5157a..a8a4821dbb 100644 > --- a/lib/librte_ethdev/rte_ethdev_version.map > +++ b/lib/librte_ethdev/rte_ethdev_version.map > @@ -229,6 +229,10 @@ EXPERIMENTAL { > # added in 20.11 > rte_eth_link_speed_to_str; > rte_eth_link_to_str; > + rte_flow_shared_action_create; > + rte_flow_shared_action_destroy; > + rte_flow_shared_action_update; > + rte_flow_shared_action_query; Shouldn't it be alphabetically sorted? > }; > > INTERNAL { > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c > index f8fdd68fe9..9afa8905df 100644 > --- a/lib/librte_ethdev/rte_flow.c > +++ b/lib/librte_ethdev/rte_flow.c > @@ -174,6 +174,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = { > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)), > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)), > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)), > + MK_FLOW_ACTION(SHARED, 0), It looks correct, it deserves a comment which explains why size is 0 here. > }; > > int > @@ -1251,3 +1252,86 @@ rte_flow_get_aged_flows(uint16_t port_id, void **contexts, > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > NULL, rte_strerror(ENOTSUP)); > } > + > +struct rte_flow_shared_action * > +rte_flow_shared_action_create(uint16_t port_id, > + const struct rte_flow_shared_action_conf *conf, > + const struct rte_flow_action *action, > + struct rte_flow_error *error) > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; Sorry, but it unsafe to initialize it before port_id check. It is too error-prone for the future maintenance of the code. Since, port_id is checked indirection here via rte_flow_ops_get(), please, initialize the variable or even move it completely to be just before usage. I realize that nearby code does the same, but IMHO it is never later to turn into the right direction. > + struct rte_flow_shared_action *shared_action; > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > + > + if (unlikely(!ops)) > + return NULL; > + if (likely(!!ops->shared_action_create)) { > + shared_action = ops->shared_action_create(dev, conf, action, > + error); > + if (shared_action == NULL) > + flow_err(port_id, -rte_errno, error); > + return shared_action; > + } > + rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, rte_strerror(ENOSYS)); > + return NULL; > +} > + > +int > +rte_flow_shared_action_destroy(uint16_t port_id, > + struct rte_flow_shared_action *action, > + struct rte_flow_error *error) > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; same here > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > + > + if (unlikely(!ops)) > + return -rte_errno; > + if (likely(!!ops->shared_action_destroy)) > + return flow_err(port_id, > + ops->shared_action_destroy(dev, action, error), > + error); > + return rte_flow_error_set(error, ENOSYS, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, rte_strerror(ENOSYS)); > +} > + > +int > +rte_flow_shared_action_update(uint16_t port_id, > + struct rte_flow_shared_action *action, > + const struct rte_flow_action *update, > + struct rte_flow_error *error) > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; same here > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > + > + if (unlikely(!ops)) > + return -rte_errno; > + if (likely(!!ops->shared_action_update)) > + return flow_err(port_id, ops->shared_action_update(dev, action, > + update, error), > + error); Sorry, but above is very hard to follow what is where. May be helper variable sfor op result should be used to make it readable. > + return rte_flow_error_set(error, ENOSYS, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, rte_strerror(ENOSYS)); > +} > + > +int > +rte_flow_shared_action_query(uint16_t port_id, > + const struct rte_flow_shared_action *action, > + void *data, > + struct rte_flow_error *error) > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; same here > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > + > + if (unlikely(!ops)) > + return -rte_errno; > + if (likely(!!ops->shared_action_query)) > + return flow_err(port_id, ops->shared_action_query(dev, action, > + data, error), > + error); Sorry, but above is very hard to follow what is where. May be helper variable sfor op result should be used to make it readable. > + return rte_flow_error_set(error, ENOSYS, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, rte_strerror(ENOSYS)); > +} > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > index da8bfa5489..9050adec23 100644 > --- a/lib/librte_ethdev/rte_flow.h > +++ b/lib/librte_ethdev/rte_flow.h > @@ -1714,7 +1714,8 @@ enum rte_flow_action_type { > /** > * Enables counters for this flow rule. > * > - * These counters can be retrieved and reset through rte_flow_query(), > + * These counters can be retrieved and reset through rte_flow_query() or > + * rte_flow_shared_action_query() if the action provided via handle, > * see struct rte_flow_query_count. > * > * See struct rte_flow_action_count. > @@ -2132,6 +2133,14 @@ enum rte_flow_action_type { > * see enum RTE_ETH_EVENT_FLOW_AGED > */ > RTE_FLOW_ACTION_TYPE_AGE, > + > + /** > + * Describe action shared across multiple flow rules. > + * > + * Allow multiple rules reference the same action by handle (see > + * struct rte_flow_shared_action). > + */ > + RTE_FLOW_ACTION_TYPE_SHARED, > }; > > /** > @@ -2693,6 +2702,20 @@ struct rte_flow_action_set_dscp { > uint8_t dscp; > }; > > + > +/** > + * RTE_FLOW_ACTION_TYPE_SHARED > + * > + * Opaque type returned after successfully creating a shared action. > + * > + * This handle can be used to manage and query the related action: > + * - share it across multiple flow rules > + * - update action configuration > + * - query action data > + * - destroy action > + */ > +struct rte_flow_shared_action; > + > /* Mbuf dynamic field offset for metadata. */ > extern int32_t rte_flow_dynf_metadata_offs; > > @@ -3357,6 +3380,150 @@ int > rte_flow_get_aged_flows(uint16_t port_id, void **contexts, > uint32_t nb_contexts, struct rte_flow_error *error); > > +/** > + * Specify shared action configuration > + */ > +struct rte_flow_shared_action_conf { > + /** > + * Flow direction for shared action configuration. > + * > + * Shred action should be valid at least for one flow direction, > + * otherwise it is invalid for both ingress and egress rules. > + */ > + uint32_t ingress:1; > + /**< Action valid for rules applied to ingress traffic. */ > + uint32_t egress:1; > + /**< Action valid for rules applied to egress traffic. */ > +}; > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Create shared action for reuse in multiple flow rules. > + * The created shared action has single state and configuration > + * across all flow rules using it. > + * > + * @param[in] port_id > + * The port identifier of the Ethernet device. > + * @param[in] conf > + * Shared action configuration. > + * @param[in] action > + * Action configuration for shared action creation. > + * @param[out] error > + * Perform verbose error reporting if not NULL. PMDs initialize this > + * structure in case of error only. > + * @return > + * A valid handle in case of success, NULL otherwise and rte_errno is set > + * to one of the error codes defined: > + * - (ENOSYS) if underlying device does not support this functionality. > + * - (EIO) if underlying device is removed. > + * - (EINVAL) if *action* invalid. > + * - (ENOTSUP) if *action* valid but unsupported. ENODEV ? > + */ > +__rte_experimental > +struct rte_flow_shared_action * > +rte_flow_shared_action_create(uint16_t port_id, > + const struct rte_flow_shared_action_conf *conf, > + const struct rte_flow_action *action, > + struct rte_flow_error *error); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Destroy the shared action by handle. > + * > + * @param[in] port_id > + * The port identifier of the Ethernet device. > + * @param[in] action > + * Handle for the shared action to be destroyed. > + * @param[out] error > + * Perform verbose error reporting if not NULL. PMDs initialize this > + * structure in case of error only. > + * @return > + * - (0) if success. > + * - (-ENOSYS) if underlying device does not support this functionality. > + * - (-EIO) if underlying device is removed. > + * - (-ENOENT) if action pointed by *action* handle was not found. > + * - (-ETOOMANYREFS) if action pointed by *action* handle still used by one or > + * more rules > + * rte_errno is also set. -ENODEV? > + */ > +__rte_experimental > +int > +rte_flow_shared_action_destroy(uint16_t port_id, > + struct rte_flow_shared_action *action, > + struct rte_flow_error *error); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Update in-place the shared action configuration pointed by *action* handle > + * with the configuration provided as *update* argument. > + * The update of the shared action configuration effects all flow rules reusing > + * the action via handle. > + * > + * @param[in] port_id > + * The port identifier of the Ethernet device. > + * @param[in] action > + * Handle for the shared action to be updated. > + * @param[in] update > + * Action specification used to modify the action pointed by handle. > + * *update* should be of same type with the action pointed by the *action* > + * handle argument, otherwise considered as invalid. > + * @param[out] error > + * Perform verbose error reporting if not NULL. PMDs initialize this > + * structure in case of error only. > + * @return > + * - (0) if success. > + * - (-ENOSYS) if underlying device does not support this functionality. > + * - (-EIO) if underlying device is removed. > + * - (-EINVAL) if *update* invalid. > + * - (-ENOTSUP) if *update* valid but unsupported. > + * - (-ENOENT) if action pointed by *ctx* was not found. -ENODEV ? > + * rte_errno is also set. > + */ > +__rte_experimental > +int > +rte_flow_shared_action_update(uint16_t port_id, > + struct rte_flow_shared_action *action, > + const struct rte_flow_action *update, > + struct rte_flow_error *error); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Query the shared action by handle. > + * > + * Retrieve action-specific data such as counters. > + * Data is gathered by special action which may be present/referenced in > + * more than one flow rule definition. > + * > + * \see RTE_FLOW_ACTION_TYPE_COUNT > + * > + * @param port_id > + * Port identifier of Ethernet device. > + * @param[in] action > + * Handle for the shared action to query. > + * @param[in, out] data > + * Pointer to storage for the associated query data type. > + * @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_shared_action_query(uint16_t port_id, > + const struct rte_flow_shared_action *action, > + void *data, > + struct rte_flow_error *error); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h > index 3ee871d3eb..adaace47ea 100644 > --- a/lib/librte_ethdev/rte_flow_driver.h > +++ b/lib/librte_ethdev/rte_flow_driver.h > @@ -108,6 +108,29 @@ struct rte_flow_ops { > void **context, > uint32_t nb_contexts, > struct rte_flow_error *err); > + /** See rte_flow_shared_action_create() */ > + struct rte_flow_shared_action *(*shared_action_create) > + (struct rte_eth_dev *dev, > + const struct rte_flow_shared_action_conf *conf, > + const struct rte_flow_action *action, > + struct rte_flow_error *error); > + /** See rte_flow_shared_action_destroy() */ > + int (*shared_action_destroy) > + (struct rte_eth_dev *dev, > + struct rte_flow_shared_action *shared_action, > + struct rte_flow_error *error); > + /** See rte_flow_shared_action_update() */ > + int (*shared_action_update) > + (struct rte_eth_dev *dev, > + struct rte_flow_shared_action *shared_action, > + const struct rte_flow_action *update, > + struct rte_flow_error *error); > + /** See rte_flow_shared_action_query() */ > + int (*shared_action_query) > + (struct rte_eth_dev *dev, > + const struct rte_flow_shared_action *shared_action, > + void *data, > + struct rte_flow_error *error); > }; > > /** >