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 84219A00BE; Thu, 17 Feb 2022 12:04:50 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2588041150; Thu, 17 Feb 2022 12:04:50 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 8EB7A4114E for ; Thu, 17 Feb 2022 12:04:48 +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)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 12A3C44; Thu, 17 Feb 2022 14:04:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 12A3C44 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1645095888; bh=kW379qeRnxSjDShdUREN08d2C+txH2ip3tK2o8+by9w=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=St3FxTeKUly031PnX5WSOeOejY7eV8IK3ekLYPnbMnk8LFJ9uBtJcZSRoUmmj/wbX 6q1opzvnWM5WDbZZAiXVDPytiB+LaFHJQJaBVqZmQRtGcR1pURxk/xFDXadMyFZ9dP geKHEwsPmVIj0PIJVlovneotivwq4RaueivvhHRY= Message-ID: <87123129-d6ca-f07a-a43d-9506dffa9ac6@oktetlabs.ru> Date: Thu, 17 Feb 2022 14:04:47 +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 v5 01/10] ethdev: introduce flow pre-configuration hints 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: <20220209213809.1208269-1-akozyrev@nvidia.com> <20220211022653.1372318-1-akozyrev@nvidia.com> <20220211022653.1372318-2-akozyrev@nvidia.com> <920abf38-3cb6-cea2-7667-f785becd0751@oktetlabs.ru> <982e92be-5e58-7e63-4bf4-7f095e45fcbd@oktetlabs.ru> <86a1de86-b218-eb8d-c58c-3c0338719f62@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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/17/22 13:57, Ori Kam wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Thursday, February 17, 2022 12:35 PM >> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration hints >> >> On 2/17/22 01:17, Alexander Kozyrev wrote: >>> On Wed, Feb 16, 2022 8:03 Andrew Rybchenko : >>>> On 2/11/22 21:47, Alexander Kozyrev wrote: >>>>> On Friday, February 11, 2022 5:17 Andrew Rybchenko >>>> wrote: >>>>>> Sent: Friday, February 11, 2022 5:17 >>>>>> To: Alexander Kozyrev ; dev@dpdk.org >>>>>> Cc: Ori Kam ; 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 >>>>>> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration >>>> hints >>>>>> >>>>>> On 2/11/22 05:26, Alexander Kozyrev wrote: >>>>>>> The flow rules creation/destruction at a large scale incurs a performance >>>>>>> penalty and may negatively impact the packet processing when used >>>>>>> as part of the datapath logic. This is mainly because software/hardware >>>>>>> resources are allocated and prepared during the flow rule creation. >>>>>>> >>>>>>> In order to optimize the insertion rate, PMD may use some hints >>>> provided >>>>>>> by the application at the initialization phase. The rte_flow_configure() >>>>>>> function allows to pre-allocate all the needed resources beforehand. >>>>>>> These resources can be used at a later stage without costly allocations. >>>>>>> Every PMD may use only the subset of hints and ignore unused ones or >>>>>>> fail in case the requested configuration is not supported. >>>>>>> >>>>>>> The rte_flow_info_get() is available to retrieve the information about >>>>>>> supported pre-configurable resources. Both these functions must be >>>> called >>>>>>> before any other usage of the flow API engine. >>>>>>> >>>>>>> Signed-off-by: Alexander Kozyrev >>>>>>> Acked-by: Ori Kam >>>> >>>> [snip] >>>> >>>>>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c >>>>>>> index a93f68abbc..66614ae29b 100644 >>>>>>> --- a/lib/ethdev/rte_flow.c >>>>>>> +++ b/lib/ethdev/rte_flow.c >>>>>>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id, >>>>>>> ret = ops->flex_item_release(dev, handle, error); >>>>>>> return flow_err(port_id, ret, error); >>>>>>> } >>>>>>> + >>>>>>> +int >>>>>>> +rte_flow_info_get(uint16_t port_id, >>>>>>> + struct rte_flow_port_info *port_info, >>>>>>> + 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(!ops)) >>>>>>> + return -rte_errno; >>>>>>> + if (likely(!!ops->info_get)) { >>>>>> >>>>>> expected ethdev state must be validated. Just configured? >>>>>> >>>>>>> + return flow_err(port_id, >>>>>>> + ops->info_get(dev, port_info, error), >>>>>> >>>>>> port_info must be checked vs NULL >>>>> >>>>> We don’t have any NULL checks for parameters in the whole ret flow API >>>> library. >>>>> See rte_flow_create() for example. attributes, pattern and actions are >>>> passed to PMD unchecked. >>>> >>>> IMHO it is hardly a good reason to have no such check here. >>>> The API is pure control path. So, it must validate all input >>>> arguments and it is better to do it in a generic place. >>> >>> Agree, I have no objections to introduce these validation checks on control path. >> >> Good, we have a progress. >> >>> My only concern is the data-path performance, so I'm reluctant to add them to >>> rte_flow_q_create/destroy functions. But let's add NULL checks to configuration >>> routines, ok? >> >> My opinion is not that strong on the aspect, but, personally, >> I'd have sanity checks in the case of flow create/destroy as >> well. First of all it is not a true datapath. Second, these >> checks are very lightweight. >> >> Anyway, if nobody supports me, I'm OK to go without these >> checks in generic functions, but it would be very useful to >> highlight it in the parameters description. >> > I vote for adding the checks only on configuration function. > From my point of view the rule functions are part of the data path > and should be treated this way. > >>>>>>> + error); >>>>>>> + } >>>>>>> + return rte_flow_error_set(error, ENOTSUP, >>>>>>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >>>>>>> + NULL, rte_strerror(ENOTSUP)); >>>>>>> +} >>>>>>> + >>>>>>> +int >>>>>>> +rte_flow_configure(uint16_t port_id, >>>>>>> + const struct rte_flow_port_attr *port_attr, >>>>>>> + 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(!ops)) >>>>>>> + return -rte_errno; >>>>>>> + if (likely(!!ops->configure)) { >>>>>> >>>>>> The API must validate ethdev state. configured and not started? >>>>> Again, we have no such validation for any rte flow API today. >>>> >>>> Same here. If documentation defines in which state the API >>>> should be called, generic code must guarantee it. >>> >>> Ok, as long as it stays in the configuration phase only. >>> >>>>>> >>>>>>> + return flow_err(port_id, >>>>>>> + ops->configure(dev, port_attr, error), >>>>>> >>>>>> port_attr must be checked vs NULL >>>>> Same. >>>>> >>>>>>> + 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 1031fb246b..92be2a9a89 100644 >>>>>>> --- a/lib/ethdev/rte_flow.h >>>>>>> +++ b/lib/ethdev/rte_flow.h >>>>>>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t >>>> port_id, >>>>>>> const struct rte_flow_item_flex_handle *handle, >>>>>>> struct rte_flow_error *error); >>>>>>> >>>>>>> +/** >>>>>>> + * @warning >>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice. >>>>>>> + * >>>>>>> + * Information about available pre-configurable resources. >>>>>>> + * The zero value means a resource cannot be pre-allocated. >>>>>>> + * >>>>>>> + */ >>>>>>> +struct rte_flow_port_info { >>>>>>> + /** >>>>>>> + * Number of pre-configurable counter actions. >>>>>>> + * @see RTE_FLOW_ACTION_TYPE_COUNT >>>>>>> + */ >>>>>>> + uint32_t nb_counters; >>>>>> >>>>>> Name says that it is a number of counters, but description >>>>>> says that it is about actions. >>>>>> Also I don't understand what does "pre-configurable" mean. >>>>>> Isn't it a maximum number of available counters? >>>>>> If no, how can I find a maximum? >>>>> It is number of pre-allocated and pre-configured actions. >>>>> How are they pr-configured is up to PDM driver. >>>>> But let's change to "pre-configured" everywhere. >>>>> Configuration includes some memory allocation anyway. >>>> >>>> Sorry, but I still don't understand. I guess HW has >>>> a hard limit on a number of counters. How can I get >>>> the information? >>> >>> Sorry for not being clear. These are resources/objects limitation. >>> It may be the hard HW limit on number of counter objects, for example. >>> Or the system has a little of memory and NIC is constrained in memory >>> in its attempt to create these counter objects as another example. >>> In any case, the info_get() API should return the limit to a user. >> >> Look. First of all it is confusing that description says >> "counter actions". I remember that we have no shared >> counters now (just shared actions), but it does not matter >> a lot. IMHO it is a bit more clear to say that it is >> a limit on a number of flow counters. I guess it better >> express the nature of the limitation. May be I'm missing >> something. If so, I'd like to understand what. >> > From my view point, this should be the number of resource/objects that > the HW can allocate (in an ideal system). > For example if the HW can allocate 1M counters but due to limited memory > on the system the actual number can be less. > > Like you said we also have the handle action, this means that > the same object can be shared between any number of rules. > as a result the limitation is not on the number of rules but on the number of > resources allocated. > > In addition and even more importantly during this stage there is no knowlge on the > number of rules that will be inserted. > > So can we agree to say resources? Yes >> Second, "per-configurable" is confusing. May be it is better >> just to drop it? I.e. "Information about available resources." >> Otherwise it is necessary to explain who and when >> pre-configures these resources. Is it really pre-configured? >> > I'm O.K with dropping the configuration part > It should just say number of counter objects > >> "The zero value means a resource cannot be pre-allocated." >> Does it mean that the action cannot be used at all? >> I think it must be explicitly clarified in the case of any >> answer. > > Agree, it should state that if the PMD report 0 in means that > it doesn’t support such an object. Good.