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 7DB9F41BAB; Thu, 2 Feb 2023 10:15:28 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5CC0B42D3D; Thu, 2 Feb 2023 10:15:28 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 8FABA40689 for ; Thu, 2 Feb 2023 10:15:26 +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 (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id E38B050; Thu, 2 Feb 2023 12:15:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru E38B050 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1675329325; bh=CgSrjCVZLLlD96q64gIe00kN+zz9jKrqV2EBboR9Ue0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VwXY3u639BLZCeBPqvER6exJsbuMvdUSDoZDPVLYJlIVb/8auwdvfFidSwTLiNdXG 0DwJ0nGk+28MBiHvCmwFGCWvgv7tg8E5rmmPc5shm+fE4VCi60gniPY87Clhrv42VK Iavb1uNV6DXYBwIaDv+VXEHVKoXFqVqQZ3/MjdqQ= Message-ID: <0a24a07c-6d36-2f5e-5f24-1501bcc41c1e@oktetlabs.ru> Date: Thu, 2 Feb 2023 12:15:25 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [PATCH v8 1/2] ethdev: add query_update sync and async function calls Content-Language: en-US To: Gregory Etelson , dev@dpdk.org Cc: matan@nvidia.com, rasland@nvidia.com, Ori Kam , Thomas Monjalon , Ferruh Yigit References: <20221221073547.988-1-getelson@nvidia.com> <20230201151646.3500-1-getelson@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20230201151646.3500-1-getelson@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 query_update is bad for summary, consider something like ethdev: add query and update sync and async API On 2/1/23 18:16, Gregory Etelson wrote: > Current API allows either query or update indirect flow action. > If indirect action must be conditionally updated according to it's > present state application must first issue action query then > analyze returned data and if needed issue update request. > When the update will be processed, action state can change and > the update can invalidate the action. > > Add `rte_flow_action_handle_query_update` function call, > and it's async version `rte_flow_async_action_handle_query_update` > to atomically query and update flow action. > > Application can control query and update order, if that is supported > by port hardware, by setting `qu_mode` parameter to > RTE_FLOW_QU_QUERY_FIRST or RTE_FLOW_QU_UPDATE_FIRST. > > Signed-off-by: Gregory Etelson [snip] > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c > index 7d0c24366c..ece40e7877 100644 > --- a/lib/ethdev/rte_flow.c > +++ b/lib/ethdev/rte_flow.c > @@ -1883,3 +1883,48 @@ rte_flow_async_action_handle_query(uint16_t port_id, > action_handle, data, user_data, error); > return flow_err(port_id, ret, error); > } > + > +int > +rte_flow_action_handle_query_update(uint16_t port_id, > + struct rte_flow_action_handle *handle, > + const void *update, void *query, > + enum rte_flow_query_update_mode mode, > + struct rte_flow_error *error) > +{ > + int ret; > + struct rte_eth_dev *dev; > + const struct rte_flow_ops *ops; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + dev = &rte_eth_devices[port_id]; (-EINVAL) if *handle* invalid or both *query* and *update* are NULL. must be handled here. It is generic checks and we should do it in a generic place to avoid duplication in PMD callbacks. > + ops = rte_flow_ops_get(port_id, error); > + if (!ops || !ops->action_handle_query_update) > + return -ENOTSUP; May be it makes sense to use single-operation callbacks if another operation input is NULL. I guess it could be convenient in some cases. Just an idea. > + ret = ops->action_handle_query_update(dev, handle, update, query, > + mode, error); > + return flow_err(port_id, ret, error); > +} > + > +int > +rte_flow_async_action_handle_query_update(uint16_t port_id, uint32_t queue_id, > + const struct rte_flow_op_attr *attr, > + struct rte_flow_action_handle *handle, > + const void *update, void *query, > + enum rte_flow_query_update_mode mode, > + void *user_data, > + struct rte_flow_error *error) > +{ > + int ret; > + struct rte_eth_dev *dev; > + const struct rte_flow_ops *ops; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + dev = &rte_eth_devices[port_id]; same here > + ops = rte_flow_ops_get(port_id, error); > + if (!ops || !ops->async_action_handle_query_update) > + return -ENOTSUP; same here > + ret = ops->async_action_handle_query_update(dev, queue_id, attr, > + handle, update, query, mode, > + user_data, error); > + return flow_err(port_id, ret, error); > +} > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > index b60987db4b..d3fbba5b99 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -5622,6 +5622,109 @@ rte_flow_async_action_handle_query(uint16_t port_id, > void *user_data, > struct rte_flow_error *error); > > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Query and update operational mode. > + * > + * RTE_FLOW_QU_QUERY_FIRST > + * Force port to query action before update. > + * RTE_FLOW_QU_UPDATE_FIRST > + * Force port to update action before query. I'm sorry, but I strongly dislike enum members duplication here. I don't understand why we need to duplicate it and why we can't document it in a right way below. > + * > + * @see rte_flow_action_handle_query_update() > + * @see rte_flow_async_action_handle_query_update() > + */ > +enum rte_flow_query_update_mode { > + RTE_FLOW_QU_QUERY_FIRST, /**< Query before update. */ > + RTE_FLOW_QU_UPDATE_FIRST, /**< Query after update. */ > +}; > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Query and/or update indirect flow action. > + * If both query and update not NULL, the function atomically > + * queries and updates indirect action. Query and update are carried in order > + * specified in the mode parameter. > + * > + * @param port_id > + * Port identifier of Ethernet device. > + * @param handle > + * Handle for the indirect action object to be updated. > + * @param update > + * If not NULL, update profile specification used to modify the action > + * pointed by handle. > + * @param query > + * If not NULL pointer to storage for the associated query data type. > + * @param mode > + * Operational mode. > + * Required if both *update* and *query* are not NULL. It should be a part of generic checks in rte_flow_action_handle_query_update() implementation. > + * @param 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. > + * - (-ENODEV) if *port_id* invalid. > + * - (-ENOTSUP) if underlying device does not support this functionality. > + * - (-EINVAL) if *handle* invalid or both *query* and *update* are NULL. > + */ > +__rte_experimental > +int > +rte_flow_action_handle_query_update(uint16_t port_id, > + struct rte_flow_action_handle *handle, > + const void *update, void *query, > + enum rte_flow_query_update_mode mode, > + struct rte_flow_error *error); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Enqueue async indirect flow action query and/or update > + * > + * @param port_id > + * Port identifier of Ethernet device. > + * @param queue_id > + * Flow queue which is used to update the rule. > + * @param attr > + * Indirect action update operation attributes. > + * @param handle > + * Handle for the indirect action object to be updated. > + * @param update > + * If not NULL, update profile specification used to modify the action > + * pointed by handle. > + * @param query > + * If not NULL, pointer to storage for the associated query data type. > + * Query result returned on async completion event. > + * @param mode > + * Operational mode. > + * Required if both *update* and *query* are not NULL. It should be a part of generic checks in rte_flow_action_handle_query_update() implementation. > + * @param user_data > + * The user data that will be returned on async completion event. > + * @param 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. > + * - (-ENODEV) if *port_id* invalid. > + * - (-ENOTSUP) if underlying device does not support this functionality. > + * - (-EINVAL) if *handle* invalid or both *update* and *query* are NULL. > + */ > +__rte_experimental > +int > +rte_flow_async_action_handle_query_update(uint16_t port_id, uint32_t queue_id, > + const struct rte_flow_op_attr *attr, > + struct rte_flow_action_handle *handle, > + const void *update, void *query, > + enum rte_flow_query_update_mode mode, > + void *user_data, > + struct rte_flow_error *error); > + > #ifdef __cplusplus > } > #endif [snip]