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 93DCD43A4E; Fri, 2 Feb 2024 10:38:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7DB88402DC; Fri, 2 Feb 2024 10:38:15 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 586B74026E for ; Fri, 2 Feb 2024 10:38:13 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 1ADC81FEFD for ; Fri, 2 Feb 2024 10:38:13 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 0CF7F1FF9E; Fri, 2 Feb 2024 10:38:13 +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 E6ABE1FF20; Fri, 2 Feb 2024 10:38:10 +0100 (CET) Message-ID: Date: Fri, 2 Feb 2024 10:38:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 11/11] eventdev: RFC clarify docs on event object fields 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-12-bruce.richardson@intel.com> <211a59b9-095d-418e-be82-b49f4e5d1d00@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-02-01 17:59, Bruce Richardson wrote: > On Wed, Jan 24, 2024 at 12:34:50PM +0100, Mattias Rönnblom wrote: >> On 2024-01-19 18:43, Bruce Richardson wrote: >>> Clarify the meaning of the NEW, FORWARD and RELEASE event types. >>> For the fields in "rte_event" struct, enhance the comments on each to >>> clarify the field's use, and whether it is preserved between enqueue and >>> dequeue, and it's role, if any, in scheduling. >>> >>> Signed-off-by: Bruce Richardson >>> --- >>> >>> As with the previous patch, please review this patch to ensure that the >>> expected semantics of the various event types and event fields have not >>> changed in an unexpected way. >>> --- >>> lib/eventdev/rte_eventdev.h | 105 ++++++++++++++++++++++++++---------- >>> 1 file changed, 77 insertions(+), 28 deletions(-) >>> >>> diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h >>> index cb13602ffb..4eff1c4958 100644 >>> --- a/lib/eventdev/rte_eventdev.h >>> +++ b/lib/eventdev/rte_eventdev.h >>> @@ -1416,21 +1416,25 @@ struct rte_event_vector { >>> >>> /* Event enqueue operations */ >>> #define RTE_EVENT_OP_NEW 0 >>> -/**< The event producers use this operation to inject a new event to the >>> +/**< The @ref rte_event.op field should be set to this type to inject a new event to the >>> * event device. >>> */ >> >> "type" -> "value" >> >> "to" -> "into"? >> >> You could also say "to mark the event as new". >> >> What is new? Maybe "new (as opposed to a forwarded) event." or "new (i.e., >> not previously dequeued).". >> > > Using this latter suggested wording in V3. > >> "The application sets the @ref rte_event.op field of an enqueued event to >> this value to mark the event as new (i.e., not previously dequeued)." >> >>> #define RTE_EVENT_OP_FORWARD 1 >>> -/**< The CPU use this operation to forward the event to different event queue or >>> - * change to new application specific flow or schedule type to enable >>> - * pipelining. >>> +/**< SW should set the @ref rte_event.op filed to this type to return a >>> + * previously dequeued event to the event device for further processing. >> >> "filed" -> "field" >> >> "SW" -> "The application" >> >> "to be scheduled for further processing (or transmission)" >> >> The wording should otherwise be the same as NEW, whatever you choose there. >> > Ack. > >>> * >>> - * This operation must only be enqueued to the same port that the >>> + * This event *must* be enqueued to the same port that the >>> * event to be forwarded was dequeued from. >> >> OK, so you "should" mark a new event RTE_EVENT_OP_FORWARD but you "*must*" >> enqueue it to the same port. >> >> I think you "must" do both. >> > Ack > >>> + * >>> + * The event's fields, including (but not limited to) flow_id, scheduling type, >>> + * destination queue, and event payload e.g. mbuf pointer, may all be updated as >>> + * desired by software, but the @ref rte_event.impl_opaque field must >> >> "software" -> "application" >> > Ack > >>> + * be kept to the same value as was present when the event was dequeued. >>> */ >>> #define RTE_EVENT_OP_RELEASE 2 >>> /**< Release the flow context associated with the schedule type. >>> * >>> - * If current flow's scheduler type method is *RTE_SCHED_TYPE_ATOMIC* >>> + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ATOMIC >>> * then this function hints the scheduler that the user has completed critical >>> * section processing in the current atomic context. >>> * The scheduler is now allowed to schedule events from the same flow from >>> @@ -1442,21 +1446,19 @@ struct rte_event_vector { >>> * performance, but the user needs to design carefully the split into critical >>> * vs non-critical sections. >>> * >>> - * If current flow's scheduler type method is *RTE_SCHED_TYPE_ORDERED* >>> - * then this function hints the scheduler that the user has done all that need >>> - * to maintain event order in the current ordered context. >>> - * The scheduler is allowed to release the ordered context of this port and >>> - * avoid reordering any following enqueues. >>> - * >>> - * Early ordered context release may increase parallelism and thus system >>> - * performance. >>> + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ORDERED >> >> Isn't a missing "or @ref RTE_SCHED_TYPE_ATOMIC" just an oversight (in the >> original API wording)? >> > > No, I don't think so, because ATOMIC is described above. > >>> + * then this function informs the scheduler that the current event has >>> + * completed processing and will not be returned to the scheduler, i.e. >>> + * it has been dropped, and so the reordering context for that event >>> + * should be considered filled. >>> * >>> - * If current flow's scheduler type method is *RTE_SCHED_TYPE_PARALLEL* >>> + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_PARALLEL >>> * or no scheduling context is held then this function may be an NOOP, >>> * depending on the implementation. >> >> Maybe you can also fix this "function" -> "operation". I suggest you delete >> that sentence, because it makes no sense. >> >> What is says in a somewhat vague manner is that you tread into the realm of >> undefined behavior if you release PARALLEL events. >> > > Agree. Just deleting. > >>> * >>> * This operation must only be enqueued to the same port that the >>> - * event to be released was dequeued from. >>> + * event to be released was dequeued from. The @ref rte_event.impl_opaque >>> + * field in the release event must match that in the original dequeued event. >> >> I would say "same value" rather than "match". >> >> "The @ref rte_event.impl_opaque field in the release event have the same >> value as in the original dequeued event." >> > Ack. > >>> */ >>> >>> /** >>> @@ -1473,53 +1475,100 @@ struct rte_event { >>> /**< Targeted flow identifier for the enqueue and >>> * dequeue operation. >>> * The value must be in the range of >>> - * [0, nb_event_queue_flows - 1] which >>> + * [0, @ref rte_event_dev_config.nb_event_queue_flows - 1] which >> >> The same comment as I had before about ranges for unsigned types. >> > Ack. > >>> * previously supplied to rte_event_dev_configure(). >>> + * >>> + * For @ref RTE_SCHED_TYPE_ATOMIC, this field is used to identify a >>> + * flow context for atomicity, such that events from each individual flow >>> + * will only be scheduled to one port at a time. >> >> flow_id alone doesn't identify an atomic flow. It's queue_id + flow_id. I'm >> not sure I think "flow context" adds much, unless it's defined somewhere. >> Sounds like some assumed implementation detail. >> > Removing the word context, and adding that it identifies a flow "within a > queue and priority level", to make it clear that it's just not the flow_id > involved here, as you say. > >>> + * >>> + * This field is preserved between enqueue and dequeue when >>> + * a device reports the @ref RTE_EVENT_DEV_CAP_CARRY_FLOW_ID >>> + * capability. Otherwise the value is implementation dependent >>> + * on dequeue > */ >>> uint32_t sub_event_type:8; >>> /**< Sub-event types based on the event source. >>> + * >>> + * This field is preserved between enqueue and dequeue. >>> + * This field is for SW or event adapter use, >> >> "SW" -> "application" >> > Ack. > >>> + * and is unused in scheduling decisions. >>> + * >>> * @see RTE_EVENT_TYPE_CPU >>> */ >>> uint32_t event_type:4; >>> - /**< Event type to classify the event source. >>> - * @see RTE_EVENT_TYPE_ETHDEV, (RTE_EVENT_TYPE_*) >>> + /**< Event type to classify the event source. (RTE_EVENT_TYPE_*) >>> + * >>> + * This field is preserved between enqueue and dequeue >>> + * This field is for SW or event adapter use, >>> + * and is unused in scheduling decisions. >> >> "unused" -> "is not considered"? >> > Ack. > >>> */ >>> uint8_t op:2; >>> - /**< The type of event enqueue operation - new/forward/ >>> - * etc.This field is not preserved across an instance >>> + /**< The type of event enqueue operation - new/forward/ etc. >>> + * >>> + * This field is *not* preserved across an instance >>> * and is undefined on dequeue. >> >> Maybe you should use "undefined" rather than "implementation dependent", or >> change this instance of undefined to implementation dependent. Now two >> different terms are used for the same thing. >> > > Using implementation dependent. > Ideally, I think we should update all drivers to set this to "FORWARD" by > default on dequeue, but for now it's "implementation dependent". > That would make a lot of sense. >>> - * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*) >>> + * >>> + * @see RTE_EVENT_OP_NEW >>> + * @see RTE_EVENT_OP_FORWARD >>> + * @see RTE_EVENT_OP_RELEASE >>> */ >>> uint8_t rsvd:4; >>> - /**< Reserved for future use */ >>> + /**< Reserved for future use. >>> + * >>> + * Should be set to zero on enqueue. Zero on dequeue. >>> + */ >> >> Why say they should be zero on dequeue? Doesn't this defeat the purpose of >> having reserved bits, for future use? With you suggested wording, you can't >> use these bits without breaking the ABI. > > Good point. Removing the dequeue value bit. > >> >>> uint8_t sched_type:2; >>> /**< Scheduler synchronization type (RTE_SCHED_TYPE_*) >>> * associated with flow id on a given event queue >>> * for the enqueue and dequeue operation. >>> + * >>> + * This field is used to determine the scheduling type >>> + * for events sent to queues where @ref RTE_EVENT_QUEUE_CFG_ALL_TYPES >>> + * is supported. >> >> "supported" -> "configured" >> > Ack. > >>> + * For queues where only a single scheduling type is available, >>> + * this field must be set to match the configured scheduling type. >>> + * >> >> Why is the API/event device asking this of the application? >> > Historical reasons. I agree that it shouldn't, this should just be marked > as ignored on fixed-type queues, but the spec up till now says it must > match and some drivers do check this. Once we update the drivers to stop > checking then we can change the spec without affecting apps. > >>> + * This field is preserved between enqueue and >>> dequeue. + * + * @see >>> RTE_SCHED_TYPE_ORDERED + * @see >>> RTE_SCHED_TYPE_ATOMIC + * @see >>> RTE_SCHED_TYPE_PARALLEL */ uint8_t queue_id; /**< Targeted event queue >>> identifier for the enqueue or * dequeue operation. * The value must be >>> in the range of - * [0, nb_event_queues - 1] which >>> previously supplied to - * >>> rte_event_dev_configure(). + * [0, @ref >>> rte_event_dev_config.nb_event_queues - 1] which was + >>> * previously supplied to rte_event_dev_configure(). + >>> * + * This field is preserved between enqueue on >>> dequeue. */ uint8_t priority; /**< Event priority relative to other >>> events in the * event queue. The requested priority should in the - >>> * range of [RTE_EVENT_DEV_PRIORITY_HIGHEST, - * >>> RTE_EVENT_DEV_PRIORITY_LOWEST]. + * range of [@ref >>> RTE_EVENT_DEV_PRIORITY_HIGHEST, + * @ref >>> RTE_EVENT_DEV_PRIORITY_LOWEST]. * The implementation shall normalize >>> the requested * priority to supported priority value. + >>> * * Valid when the device has - * >>> RTE_EVENT_DEV_CAP_EVENT_QOS capability. + * @ref >>> RTE_EVENT_DEV_CAP_EVENT_QOS capability. + * Ignored >>> otherwise. + * + * This >>> field is preserved between enqueue and dequeue. >> >> Is the normalized or unnormalized value that is preserved? >> > Very good point. It's the normalized & then denormalised version that is > guaranteed to be preserved, I suspect. SW eventdevs probably preserve > as-is, but HW eventdevs may lose precision. Rather than making this > "implementation defined" or "not preserved" which would be annoying for > apps, I think, I'm going to document this as "preserved, but with possible > loss of precision". > This makes me again think it may be worth noting that Eventdev -> API priority normalization is (event.priority * PMD_LEVELS) / EVENTDEV_LEVELS (rounded down) - assuming that's how it's supposed to be done - or something to that effect. >>> */ >>> uint8_t impl_opaque; >>> /**< Implementation specific opaque value. >> >> Maybe you can also fix "implementation" here to be something more specific. >> Implementation, of what? >> >> "Event device implementation" or just "event device". >> > "Opaque field for event device use" > >>> + * >>> * An implementation may use this field to hold >>> * implementation specific value to share between >>> * dequeue and enqueue operation. >>> + * >>> * The application should not modify this field. >>> + * Its value is implementation dependent on dequeue, >>> + * and must be returned unmodified on enqueue when >>> + * op type is @ref RTE_EVENT_OP_FORWARD or @ref RTE_EVENT_OP_RELEASE >> >> Should it be mentioned that impl_opaque is ignored by the event device for >> NEW events? >> > Added in V3. > >>> */ >>> }; >>> }; >>> -- >>> 2.40.1 >>>