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 713A5A034E; Mon, 21 Feb 2022 15:49:08 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A6214068C; Mon, 21 Feb 2022 15:49:08 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 42CA44013F for ; Mon, 21 Feb 2022 15:49:06 +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 A6A4838; Mon, 21 Feb 2022 17:49:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A6A4838 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645454945; bh=EHFoqLhfFSnkgrN1GFe2ynrUZJ42VCikMZVF9zJR1dM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=QwIqWiFore1wSIrSAAYFlpGCxXOueryzTfgqu3reBcm8/OMZKGTEewJuDTs8E61VT 9mWeXtp1Zy5porVb+sc1ohktdyxp6weGVNC1snMQmsvzQ7lgtyitwmv8jhNrfrjZOK mWRifV4PJfT9SA9jfftS73kGh6HlDJsiMmKHf5KM= Message-ID: <207e5ed4-f337-08fd-d7cf-8a821bb897d9@oktetlabs.ru> Date: Mon, 21 Feb 2022 17:49:05 +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 03/11] ethdev: bring in async queue-based flow rules operations 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-4-akozyrev@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220220034409.2226860-4-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: > A new, faster, queue-based flow rules management mechanism is needed for > applications offloading rules inside the datapath. This asynchronous > and lockless mechanism frees the CPU for further packet processing and > reduces the performance impact of the flow rules creation/destruction > on the datapath. Note that queues are not thread-safe and the queue > should be accessed from the same thread for all queue operations. > It is the responsibility of the app to sync the queue functions in case > of multi-threaded access to the same queue. > > The rte_flow_async_create() function enqueues a flow creation to the > requested queue. It benefits from already configured resources and sets > unique values on top of item and action templates. A flow rule is enqueued > on the specified flow queue and offloaded asynchronously to the hardware. > The function returns immediately to spare CPU for further packet > processing. The application must invoke the rte_flow_pull() function > to complete the flow rule operation offloading, to clear the queue, and to > receive the operation status. The rte_flow_async_destroy() function > enqueues a flow destruction to the requested queue. > > Signed-off-by: Alexander Kozyrev > Acked-by: Ori Kam [snip] > @@ -3777,6 +3782,125 @@ and pattern and actions templates are created. > &actions_templates, nb_actions_templ, > &error); > > +Asynchronous operations > +----------------------- > + > +Flow rules management can be done via special lockless flow management queues. > +- Queue operations are asynchronous and not thread-safe. > + > +- Operations can thus be invoked by the app's datapath, > + packet processing can continue while queue operations are processed by NIC. > + > +- Number of flow queues is configured at initialization stage. > + > +- Available operation types: rule creation, rule destruction, > + indirect rule creation, indirect rule destruction, indirect rule update. > + > +- Operations may be reordered within a queue. > + > +- Operations can be postponed and pushed to NIC in batches. > + > +- Results pulling must be done on time to avoid queue overflows. I guess the documenation is for applications, but IMHO it is a driver responsiblity. Application should not care about it. Yes, applicatoin should do pulling, but it should not think about overflow. Request should be rejected if there is no space in queue. > + > +- User data is returned as part of the result to identify an operation. Also "User data should uniquelly identify request (may be except corner case when only one request is enqueued at most)." > + > +- Flow handle is valid once the creation operation is enqueued and must be > + destroyed even if the operation is not successful and the rule is not inserted. > + > +- Application must wait for the creation operation result before enqueueing > + the deletion operation to make sure the creation is processed by NIC. > + [snip] > +The asynchronous flow rule insertion logic can be broken into two phases. > + > +1. Initialization stage as shown here: > + > +.. _figure_rte_flow_async_init: > + > +.. figure:: img/rte_flow_async_init.* > + > +2. Main loop as presented on a datapath application example: > + > +.. _figure_rte_flow_async_usage: > + > +.. figure:: img/rte_flow_async_usage.* > + > +Enqueue creation operation > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +Enqueueing a flow rule creation operation is similar to simple creation. > + > +.. code-block:: c > + > + struct rte_flow * > + rte_flow_async_create(uint16_t port_id, > + uint32_t queue_id, > + const struct rte_flow_q_ops_attr *q_ops_attr, May be rte_flow_async_ops_attr *attr? > + struct rte_flow_template_table *template_table, > + const struct rte_flow_item pattern[], > + uint8_t pattern_template_index, > + const struct rte_flow_action actions[], > + uint8_t actions_template_index, > + void *user_data, > + struct rte_flow_error *error); > + > +A valid handle in case of success is returned. It must be destroyed later > +by calling ``rte_flow_async_destroy()`` even if the rule is rejected by HW. [snip] > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c > index e9f684eedb..4e7b202522 100644 > --- a/lib/ethdev/rte_flow.c > +++ b/lib/ethdev/rte_flow.c > @@ -1396,6 +1396,7 @@ rte_flow_flex_item_release(uint16_t port_id, > int > rte_flow_info_get(uint16_t port_id, > struct rte_flow_port_info *port_info, > + struct rte_flow_queue_info *queue_info, It should be either optional (update description) or sanity checked vs NULL below (similar to port_info). > struct rte_flow_error *error) > { > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > @@ -1415,7 +1416,7 @@ rte_flow_info_get(uint16_t port_id, > return -rte_errno; > if (likely(!!ops->info_get)) { > return flow_err(port_id, > - ops->info_get(dev, port_info, error), > + ops->info_get(dev, port_info, queue_info, error), > error); > } > return rte_flow_error_set(error, ENOTSUP, > @@ -1426,6 +1427,8 @@ rte_flow_info_get(uint16_t port_id, > int > rte_flow_configure(uint16_t port_id, > const struct rte_flow_port_attr *port_attr, > + uint16_t nb_queue, > + const struct rte_flow_queue_attr *queue_attr[], Is it really an array of pointers? If yes, why? > struct rte_flow_error *error) > { > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > @@ -1433,7 +1436,7 @@ rte_flow_configure(uint16_t port_id, > int ret; > > dev->data->flow_configured = 0; > - if (port_attr == NULL) { > + if (port_attr == NULL || queue_attr == NULL) { > RTE_FLOW_LOG(ERR, "Port %"PRIu16" info is NULL.\n", port_id); Log message becomes misleading [snip] > return -EINVAL; > } > @@ -1452,7 +1455,7 @@ rte_flow_configure(uint16_t port_id, > if (unlikely(!ops)) > return -rte_errno; > if (likely(!!ops->configure)) { > - ret = ops->configure(dev, port_attr, error); > + ret = ops->configure(dev, port_attr, nb_queue, queue_attr, error); > if (ret == 0) > dev->data->flow_configured = 1; > return flow_err(port_id, ret, error); > @@ -1713,3 +1716,104 @@ rte_flow_template_table_destroy(uint16_t port_id, > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > NULL, rte_strerror(ENOTSUP)); > } > + > +struct rte_flow * > +rte_flow_async_create(uint16_t port_id, > + uint32_t queue_id, > + const struct rte_flow_q_ops_attr *q_ops_attr, > + struct rte_flow_template_table *template_table, > + const struct rte_flow_item pattern[], > + uint8_t pattern_template_index, > + const struct rte_flow_action actions[], > + uint8_t actions_template_index, > + void *user_data, > + 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 *flow; > + > + if (unlikely(!ops)) > + return NULL; > + if (likely(!!ops->async_create)) { Hm, we should make a consistent decision. If it is super- critical fast path - we should have no sanity checks at all. If no, we should have all simple sanity checks. Otherwise, I don't understand why we do some checks and ignore another. [snip] > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > index 776e8ccc11..9e71a576f6 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -4884,6 +4884,10 @@ rte_flow_flex_item_release(uint16_t port_id, > * > */ > struct rte_flow_port_info { > + /** > + * Maximum umber of queues for asynchronous operations. umber -> number [snip]