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 56F6043A4E; Fri, 2 Feb 2024 10:24:58 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 48A1F42DDE; Fri, 2 Feb 2024 10:24:58 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id D4C6C42E27 for ; Fri, 2 Feb 2024 10:24:56 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 75B211FDDB for ; Fri, 2 Feb 2024 10:24:56 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 695B71FEB8; Fri, 2 Feb 2024 10:24:56 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.4 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.4 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id C08511FE37; Fri, 2 Feb 2024 10:24:54 +0100 (CET) Message-ID: <839761ae-ee75-4a77-8f00-f0848ff638a4@lysator.liu.se> Date: Fri, 2 Feb 2024 10:24:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 04/11] eventdev: cleanup doxygen comments on info structure Content-Language: en-US To: Bruce Richardson Cc: dev@dpdk.org, jerinj@marvell.com, mattias.ronnblom@ericsson.com, abdullah.sevincer@intel.com, sachin.saxena@oss.nxp.com, hemant.agrawal@nxp.com, pbhagavatula@marvell.com, pravin.pathak@intel.com References: <20240118134557.73172-1-bruce.richardson@intel.com> <20240119174346.108905-1-bruce.richardson@intel.com> <20240119174346.108905-5-bruce.richardson@intel.com> <65d09512-f184-4ac4-a513-e5820754889e@lysator.liu.se> <26f7d4b7-d46f-461b-a891-7a32d14f4b21@lysator.liu.se> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 2024-01-31 15:37, Bruce Richardson wrote: > On Wed, Jan 24, 2024 at 12:51:03PM +0100, Mattias Rönnblom wrote: >> On 2024-01-23 10:43, Bruce Richardson wrote: >>> On Tue, Jan 23, 2024 at 10:35:02AM +0100, Mattias Rönnblom wrote: >>>> On 2024-01-19 18:43, Bruce Richardson wrote: >>>>> Some small rewording changes to the doxygen comments on struct >>>>> rte_event_dev_info. >>>>> >>>>> Signed-off-by: Bruce Richardson >>>>> --- >>>>> lib/eventdev/rte_eventdev.h | 46 ++++++++++++++++++++----------------- >>>>> 1 file changed, 25 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h >>>>> index 57a2791946..872f241df2 100644 >>>>> --- a/lib/eventdev/rte_eventdev.h >>>>> +++ b/lib/eventdev/rte_eventdev.h >>>>> @@ -482,54 +482,58 @@ struct rte_event_dev_info { >>>>> const char *driver_name; /**< Event driver name */ >>>>> struct rte_device *dev; /**< Device information */ >>>>> uint32_t min_dequeue_timeout_ns; >>>>> - /**< Minimum supported global dequeue timeout(ns) by this device */ >>>>> + /**< Minimum global dequeue timeout(ns) supported by this device */ >>>> >>>> Are we missing a bunch of "." here and in the other fields? >>>> >>>>> uint32_t max_dequeue_timeout_ns; >>>>> - /**< Maximum supported global dequeue timeout(ns) by this device */ >>>>> + /**< Maximum global dequeue timeout(ns) supported by this device */ >>>>> uint32_t dequeue_timeout_ns; >>>>> /**< Configured global dequeue timeout(ns) for this device */ >>>>> uint8_t max_event_queues; >>>>> - /**< Maximum event_queues supported by this device */ >>>>> + /**< Maximum event queues supported by this device */ >>>>> uint32_t max_event_queue_flows; >>>>> - /**< Maximum supported flows in an event queue by this device*/ >>>>> + /**< Maximum number of flows within an event queue supported by this device*/ >>>>> uint8_t max_event_queue_priority_levels; >>>>> /**< Maximum number of event queue priority levels by this device. >>>>> - * Valid when the device has RTE_EVENT_DEV_CAP_QUEUE_QOS capability >>>>> + * Valid when the device has RTE_EVENT_DEV_CAP_QUEUE_QOS capability. >>>>> + * The priority levels are evenly distributed between >>>>> + * @ref RTE_EVENT_DEV_PRIORITY_HIGHEST and @ref RTE_EVENT_DEV_PRIORITY_LOWEST. >>>> >>>> This is a change of the API, in the sense it's defining something previously >>>> left undefined? >>>> >>> >>> Well, undefined is pretty useless for app writers, no? >>> However, agreed that the range between HIGHEST and LOWEST is an assumption >>> on my part, chosen because it matches what happens to the event priorities >>> which are documented in event struct as "The implementation shall normalize >>> the requested priority to supported priority value" - which, while better >>> than nothing, does technically leave the details of how normalization >>> occurs up to the implementation. >>> >>>> If you need 6 different priority levels in an app, how do you go about >>>> making sure you find the correct (distinct) Eventdev levels on any event >>>> device supporting >= 6 levels? >>>> >>>> #define NUM_MY_LEVELS 6 >>>> >>>> #define MY_LEVEL_TO_EVENTDEV_LEVEL(my_level) (((my_level) * >>>> (RTE_EVENT_DEV_PRIORITY_HIGHEST-RTE_EVENT_DEV_PRIORTY_LOWEST) / >>>> NUM_MY_LEVELS) >>>> >>>> This way? One would worry a bit exactly what "evenly" means, in terms of >>>> rounding errors. If you have an event device with 255 priority levels of >>>> (say) 256 levels available in the API, which two levels are the same >>>> priority? >>> >>> Yes, round etc. will be an issue in cases of non-powers-of 2. >>> However, I think we do need to clarify this behaviour, so I'm open to >>> alternative suggestions as to how update this. >>> >> >> In retrospect, maybe it would have been better to just express the number of >> priority levels an event device supported, only allow [0, max_levels - 1] in >> the prio field, and leave it to the app to do the conversion/normalization. >> > > Yes, in many ways that would be better. > >> Maybe a new helper macro would at least suggest to the PMD >> driver implementer and the application designer how this normalization >> should work. Something like the above, but where NUM_MY_LEVELS is an input >> parameter. Would result in an integer division though, so shouldn't be used >> in the fast path. > > I think it's actually ok now, having the drivers do the work, since each > driver can choose optimal method. For those having power-of-2 number of > priorities, just a shift op works best. > I had an application-usable macro in mind, not a macro for PMDs. Showing how app-level priority levels should map to Eventdev API-level priority levels would, by implication, show how event device should do the Eventdev API priority -> PMD level priority compression. The event device has exactly zero freedom in choosing how to translate Eventdev API-level priorities to its internal priorities, or risk not differentiating between app-level priority levels. If an event device supports 128 levels, is RTE_EVENT_DEV_PRIORITY_NORMAL and RTE_EVENT_DEV_PRIORITY_NORMAL-1 the same PMD-level priority or not? I would *guess* the same, but that just an assumption, and not something that follows from "normalize", I think. Anyway, this is not a problem this patch set necessarily needs to solve. > The key thing for the documentation here, to my mind, is to make it clear > that though the number of priorities is N, you still specify the relative > priorities in the range of 0-255. This is documented in the queue > configuration, so, while we could leave it unmentionned here, I think for > clarity it should be called out. I'm going to reword slightly as: > > * The implementation shall normalize priority values specified between > * @ref RTE_EVENT_DEV_PRIORITY_HIGHEST and @ref RTE_EVENT_DEV_PRIORITY_LOWEST > * to map them internally to this range of priorities. > * > * @see rte_event_queue_conf.priority > > This way the wording in the two places is similar. > > /Bruce