[v3,10/11] eventdev: clarify docs on event object fields and op types

  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 <bruce.richardson@intel.com>
---
V3: updates following review
---
 lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
 1 file changed, 111 insertions(+), 50 deletions(-)
  

On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
<bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> ---
> V3: updates following review
> ---
>  lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
>  1 file changed, 111 insertions(+), 50 deletions(-)
>
> diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> index 8d72765ae7..58219e027e 100644
> --- a/lib/eventdev/rte_eventdev.h
> +++ b/lib/eventdev/rte_eventdev.h
> @@ -1463,47 +1463,54 @@ 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
> - * event device.
> +/**< The @ref rte_event.op field must be set to this operation type to inject a new event,
> + * i.e. one not previously dequeued, into the event device, to be scheduled
> + * for processing.
>   */
>  #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.
> +/**< The application must set the @ref rte_event.op field to this operation type to return a
> + * previously dequeued event to the event device to be scheduled for further processing.
>   *
> - * 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.
> + *
> + * 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 the application, but the @ref rte_event.impl_opaque field must
> + * 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*
> - * 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
> - * an event queue to another port. However, the context may be still held
> - * until the next rte_event_dequeue_burst() call, this call allows but does not
> - * force the scheduler to release the context early.
> - *
> - * Early atomic context release may increase parallelism and thus system
> + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ATOMIC
> + * then this operation type hints the scheduler that the user has completed critical
> + * section processing for this event in the current atomic context, and that the
> + * scheduler may unlock any atomic locks held for this event.
> + * If this is the last event from an atomic flow, i.e. all flow locks are released,


Similar comment as other email
[Jerin] When there are multiple atomic events dequeue from @ref
rte_event_dequeue_burst()
for the same event queue, and it has same flow id then the lock is ....

[Mattias]
Yes, or maybe describing the whole lock/unlock state.

"The conceptual per-queue-per-flow lock is in a locked state as long
(and only as long) as one or more events pertaining to that flow were
scheduled to the port in question, but are not yet released."

Maybe it needs to be more meaty, describing what released means. I don't
have the full context of the documentation in my head when I'm writing this.







> + * the scheduler is now allowed to schedule events from that flow from to another port.
> + * However, the atomic locks may be still held until the next rte_event_dequeue_burst()
> + * call; enqueuing an event with opt type @ref RTE_EVENT_OP_RELEASE allows,

Is ";" intended?

> + * but does not force, the scheduler to release the atomic locks early.

instead of "not force", can use the term _hint_ the driver and reword.

> + *
> + * Early atomic lock release may increase parallelism and thus system
>   * 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
> + * then this operation type 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*
> - * or no scheduling context is held then this function may be an NOOP,
> - * depending on the implementation.
> + * Events with this operation type must only be enqueued to the same port that the
> + * event to be released was dequeued from. The @ref rte_event.impl_opaque
> + * field in the release event must have the same value as that in the original dequeued event.
>   *
> - * This operation must only be enqueued to the same port that the
> - * event to be released was dequeued from.
> + * If a dequeued event is re-enqueued with operation type of @ref RTE_EVENT_OP_RELEASE,
> + * then any subsequent enqueue of that event - or a copy of it - must be done as event of type
> + * @ref RTE_EVENT_OP_NEW, not @ref RTE_EVENT_OP_FORWARD. This is because any context for
> + * the originally dequeued event, i.e. atomic locks, or reorder buffer entries, will have
> + * been removed or invalidated by the release operation.
>   */
>
>  /**
> @@ -1517,56 +1524,110 @@ struct rte_event {
>                 /** Event attributes for dequeue or enqueue operation */
>                 struct {
>                         uint32_t flow_id:20;
> -                       /**< Targeted flow identifier for the enqueue and
> -                        * dequeue operation.
> -                        * The value must be in the range of
> -                        * [0, nb_event_queue_flows - 1] which
> -                        * previously supplied to rte_event_dev_configure().
> +                       /**< Target flow identifier for the enqueue and dequeue operation.
> +                        *
> +                        * For @ref RTE_SCHED_TYPE_ATOMIC, this field is used to identify a
> +                        * flow for atomicity within a queue & priority level, such that events
> +                        * from each individual flow will only be scheduled to one port at a time.
> +                        *
> +                        * 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 application or event adapter use,
> +                        * and is not considered in scheduling decisions.


cnxk driver is considering this for scheduling decision to
differentiate the producer i.e event adapters.
If other drivers are not then we can change the language around it is
implementation defined.


> +                        *
>                          * @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 application or event adapter use,
> +                        * and is not considered in scheduling decisions.


cnxk driver is considering this for scheduling decision to
differentiate the producer i.e event adapters.
If other drivers are not then we can change the language around it is
implementation defined.

>                          */
>                         uint8_t op:2;
> -                       /**< The type of event enqueue operation - new/forward/
> -                        * etc.This field is not preserved across an instance
> -                        * and is undefined on dequeue.
> -                        * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> +                       /**< The type of event enqueue operation - new/forward/ etc.
> +                        *
> +                        * This field is *not* preserved across an instance
> +                        * and is implementation dependent on dequeue.
> +                        *
> +                        * @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.

I am worried about some application explicitly start setting this to
zero on every enqueue.
Instead, can we say application should not touch the field, Since every eventdev
operations starts with dequeue() driver can fill to the correct value.

> +                        */
>                         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 configured.
> +                        * For queues where only a single scheduling type is available,
> +                        * this field must be set to match the configured scheduling type.
> +                        *
> +                        * 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().
> +                        * The value must be less than @ref rte_event_dev_config.nb_event_queues
> +                        * which was previously supplied to rte_event_dev_configure().

Some reason, similar text got removed for flow_id. Please add the same.


> +                        *
> +                        * 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.
> +                        * [For devices with where the supported priority range is a power-of-2, the
> +                        * normalization will be done via bit-shifting, so only the highest
> +                        * log2(num_priorities) bits will be used by the event device]
> +                        *
> +                        * Valid when the device has @ref RTE_EVENT_DEV_CAP_EVENT_QOS capability
> +                        * and this field is preserved between enqueue and dequeue,
> +                        * though with possible loss of precision due to normalization and
> +                        * subsequent de-normalization. (For example, if a device only supports 8
> +                        * priority levels, only the high 3 bits of this field will be
> +                        * used by that device, and hence only the value of those 3 bits are
> +                        * guaranteed to be preserved between enqueue and dequeue.)
> +                        *
> +                        * Ignored when device does not support @ref RTE_EVENT_DEV_CAP_EVENT_QOS
> +                        * capability, and it is implementation dependent if this field is preserved
> +                        * between enqueue and dequeue.
>                          */
>                         uint8_t impl_opaque;
> -                       /**< Implementation specific opaque value.
> -                        * An implementation may use this field to hold
> +                       /**< Opaque field for event device use.
> +                        *
> +                        * An event driver implementation may use this field to hold an
>                          * implementation specific value to share between
>                          * dequeue and enqueue operation.
> -                        * The application should not modify this field.
> +                        *
> +                        * The application most not modify this field.

most -> must

> +                        * 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.
> +                        * This field is ignored on events with op type
> +                        * @ref RTE_EVENT_OP_NEW.
>                          */
>                 };
>         };
> --
> 2.40.1
>
  

On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote:
> On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
> <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > ---
> > V3: updates following review
> > ---
> >  lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
> >  1 file changed, 111 insertions(+), 50 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> > index 8d72765ae7..58219e027e 100644
> > --- a/lib/eventdev/rte_eventdev.h
> > +++ b/lib/eventdev/rte_eventdev.h
> > @@ -1463,47 +1463,54 @@ 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
> > - * event device.
> > +/**< The @ref rte_event.op field must be set to this operation type to inject a new event,
> > + * i.e. one not previously dequeued, into the event device, to be scheduled
> > + * for processing.
> >   */
> >  #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.
> > +/**< The application must set the @ref rte_event.op field to this operation type to return a
> > + * previously dequeued event to the event device to be scheduled for further processing.
> >   *
> > - * 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.
> > + *
> > + * 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 the application, but the @ref rte_event.impl_opaque field must
> > + * 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*
> > - * 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
> > - * an event queue to another port. However, the context may be still held
> > - * until the next rte_event_dequeue_burst() call, this call allows but does not
> > - * force the scheduler to release the context early.
> > - *
> > - * Early atomic context release may increase parallelism and thus system
> > + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ATOMIC
> > + * then this operation type hints the scheduler that the user has completed critical
> > + * section processing for this event in the current atomic context, and that the
> > + * scheduler may unlock any atomic locks held for this event.
> > + * If this is the last event from an atomic flow, i.e. all flow locks are released,
> 
> 
> Similar comment as other email
> [Jerin] When there are multiple atomic events dequeue from @ref
> rte_event_dequeue_burst()
> for the same event queue, and it has same flow id then the lock is ....
> 
> [Mattias]
> Yes, or maybe describing the whole lock/unlock state.
> 
> "The conceptual per-queue-per-flow lock is in a locked state as long
> (and only as long) as one or more events pertaining to that flow were
> scheduled to the port in question, but are not yet released."
> 
> Maybe it needs to be more meaty, describing what released means. I don't
> have the full context of the documentation in my head when I'm writing this.
>

Will take a look to reword a bit
 
> 
> > + * the scheduler is now allowed to schedule events from that flow from to another port.
> > + * However, the atomic locks may be still held until the next rte_event_dequeue_burst()
> > + * call; enqueuing an event with opt type @ref RTE_EVENT_OP_RELEASE allows,
> 
> Is ";" intended?
> 
> > + * but does not force, the scheduler to release the atomic locks early.
> 
> instead of "not force", can use the term _hint_ the driver and reword.

Ok.
> 
> > + *
> > + * Early atomic lock release may increase parallelism and thus system
> >   * 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
> > + * then this operation type 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*
> > - * or no scheduling context is held then this function may be an NOOP,
> > - * depending on the implementation.
> > + * Events with this operation type must only be enqueued to the same port that the
> > + * event to be released was dequeued from. The @ref rte_event.impl_opaque
> > + * field in the release event must have the same value as that in the original dequeued event.
> >   *
> > - * This operation must only be enqueued to the same port that the
> > - * event to be released was dequeued from.
> > + * If a dequeued event is re-enqueued with operation type of @ref RTE_EVENT_OP_RELEASE,
> > + * then any subsequent enqueue of that event - or a copy of it - must be done as event of type
> > + * @ref RTE_EVENT_OP_NEW, not @ref RTE_EVENT_OP_FORWARD. This is because any context for
> > + * the originally dequeued event, i.e. atomic locks, or reorder buffer entries, will have
> > + * been removed or invalidated by the release operation.
> >   */
> >
> >  /**
> > @@ -1517,56 +1524,110 @@ struct rte_event {
> >                 /** Event attributes for dequeue or enqueue operation */
> >                 struct {
> >                         uint32_t flow_id:20;
> > -                       /**< Targeted flow identifier for the enqueue and
> > -                        * dequeue operation.
> > -                        * The value must be in the range of
> > -                        * [0, nb_event_queue_flows - 1] which
> > -                        * previously supplied to rte_event_dev_configure().
> > +                       /**< Target flow identifier for the enqueue and dequeue operation.
> > +                        *
> > +                        * For @ref RTE_SCHED_TYPE_ATOMIC, this field is used to identify a
> > +                        * flow for atomicity within a queue & priority level, such that events
> > +                        * from each individual flow will only be scheduled to one port at a time.
> > +                        *
> > +                        * 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 application or event adapter use,
> > +                        * and is not considered in scheduling decisions.
> 
> 
> cnxk driver is considering this for scheduling decision to
> differentiate the producer i.e event adapters.
> If other drivers are not then we can change the language around it is
> implementation defined.
> 
How does the event type influence the scheduling decision? I can drop the
last line here, but it seems strange to me that the type of event could affect
things. I would have thought that with the eventdev API only the queue,
flow id, and priority would be factors in scheduling?

> 
> > +                        *
> >                          * @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 application or event adapter use,
> > +                        * and is not considered in scheduling decisions.
> 
> 
> cnxk driver is considering this for scheduling decision to
> differentiate the producer i.e event adapters.
> If other drivers are not then we can change the language around it is
> implementation defined.
> 
> >                          */
> >                         uint8_t op:2;
> > -                       /**< The type of event enqueue operation - new/forward/
> > -                        * etc.This field is not preserved across an instance
> > -                        * and is undefined on dequeue.
> > -                        * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> > +                       /**< The type of event enqueue operation - new/forward/ etc.
> > +                        *
> > +                        * This field is *not* preserved across an instance
> > +                        * and is implementation dependent on dequeue.
> > +                        *
> > +                        * @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.
> 
> I am worried about some application explicitly start setting this to
> zero on every enqueue.
> Instead, can we say application should not touch the field, Since every eventdev
> operations starts with dequeue() driver can fill to the correct value.
> 

I'll set this to zero on "NEW", or untouched on FORWARD/RELEASE. 
If we don't state that it should be zeroed on NEW or untouched
otherwise we cannot use the space in future without ABI break.

> > +                        */
> >                         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 configured.
> > +                        * For queues where only a single scheduling type is available,
> > +                        * this field must be set to match the configured scheduling type.
> > +                        *
> > +                        * 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().
> > +                        * The value must be less than @ref rte_event_dev_config.nb_event_queues
> > +                        * which was previously supplied to rte_event_dev_configure().
> 
> Some reason, similar text got removed for flow_id. Please add the same.
> 

That was deliberate based on discussion on V2. See:

http://inbox.dpdk.org/dev/Zby3nb4NGs8T5odL@bricha3-MOBL.ger.corp.intel.com/

and wider thread discussion starting here:

http://inbox.dpdk.org/dev/ZbvOtAEpzja0gu7b@bricha3-MOBL.ger.corp.intel.com/

Basically, the comment is wrong based on what the code does now. No event
adapters or apps are limiting the flow-id, and nothing seems broken, so we
can remove the comment.
  

On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote:
> On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
> <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > ---
> > V3: updates following review
> > ---
> >  lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
> >  1 file changed, 111 insertions(+), 50 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h

<snip>

> > + * the scheduler is now allowed to schedule events from that flow from to another port.
> > + * However, the atomic locks may be still held until the next rte_event_dequeue_burst()
> > + * call; enqueuing an event with opt type @ref RTE_EVENT_OP_RELEASE allows,
> 
> Is ";" intended?

Yes, it was. :-)

/Bruce
  

On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote:
> On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
> <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > ---
> > V3: updates following review
> > ---
> >  lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
> >  1 file changed, 111 insertions(+), 50 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> > index 8d72765ae7..58219e027e 100644
> > --- a/lib/eventdev/rte_eventdev.h
> > +++ b/lib/eventdev/rte_eventdev.h
> > @@ -1463,47 +1463,54 @@ 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
> > - * event device.
> > +/**< The @ref rte_event.op field must be set to this operation type to inject a new event,
> > + * i.e. one not previously dequeued, into the event device, to be scheduled
> > + * for processing.
> >   */
> >  #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.
> > +/**< The application must set the @ref rte_event.op field to this operation type to return a
> > + * previously dequeued event to the event device to be scheduled for further processing.
> >   *
> > - * 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.
> > + *
> > + * 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 the application, but the @ref rte_event.impl_opaque field must
> > + * 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*
> > - * 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
> > - * an event queue to another port. However, the context may be still held
> > - * until the next rte_event_dequeue_burst() call, this call allows but does not
> > - * force the scheduler to release the context early.
> > - *
> > - * Early atomic context release may increase parallelism and thus system
> > + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ATOMIC
> > + * then this operation type hints the scheduler that the user has completed critical
> > + * section processing for this event in the current atomic context, and that the
> > + * scheduler may unlock any atomic locks held for this event.
> > + * If this is the last event from an atomic flow, i.e. all flow locks are released,
> 
> 
> Similar comment as other email
> [Jerin] When there are multiple atomic events dequeue from @ref
> rte_event_dequeue_burst()
> for the same event queue, and it has same flow id then the lock is ....
> 
> [Mattias]
> Yes, or maybe describing the whole lock/unlock state.
> 
> "The conceptual per-queue-per-flow lock is in a locked state as long
> (and only as long) as one or more events pertaining to that flow were
> scheduled to the port in question, but are not yet released."
> 
> Maybe it needs to be more meaty, describing what released means. I don't
> have the full context of the documentation in my head when I'm writing this.
> 
Rather than trying to explain all again, I'm just going to put inline here a
cross-reference to the text on RTE_EVENT_TYPE_ATOMIC.

/Bruce
  

On Tue, Feb 20, 2024 at 11:09 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote:
> > On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
> > <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > > ---
> > > V3: updates following review
> > > ---
> > >  lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++-----------
> > >  1 file changed, 111 insertions(+), 50 deletions(-)
> > >
> > > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> > > index 8d72765ae7..58219e027e 100644
> > > --- a/lib/eventdev/rte_eventdev.h
> > > +++ b/lib/eventdev/rte_eventdev.h
> > > @@ -1463,47 +1463,54 @@ 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
> > > - * event device.
> > > +/**< The @ref rte_event.op field must be set to this operation type to inject a new event,
> > > + * i.e. one not previously dequeued, into the event device, to be scheduled
> > > + * for processing.
> > >   */
> > >  #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.
> > > +/**< The application must set the @ref rte_event.op field to this operation type to return a
> > > + * previously dequeued event to the event device to be scheduled for further processing.
> > >   *
> > > - * 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.
> > > + *
> > > + * 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 the application, but the @ref rte_event.impl_opaque field must
> > > + * 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*
> > > - * 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
> > > - * an event queue to another port. However, the context may be still held
> > > - * until the next rte_event_dequeue_burst() call, this call allows but does not
> > > - * force the scheduler to release the context early.
> > > - *
> > > - * Early atomic context release may increase parallelism and thus system
> > > + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_ATOMIC
> > > + * then this operation type hints the scheduler that the user has completed critical
> > > + * section processing for this event in the current atomic context, and that the
> > > + * scheduler may unlock any atomic locks held for this event.
> > > + * If this is the last event from an atomic flow, i.e. all flow locks are released,
> >
> >
> > Similar comment as other email
> > [Jerin] When there are multiple atomic events dequeue from @ref
> > rte_event_dequeue_burst()
> > for the same event queue, and it has same flow id then the lock is ....
> >
> > [Mattias]
> > Yes, or maybe describing the whole lock/unlock state.
> >
> > "The conceptual per-queue-per-flow lock is in a locked state as long
> > (and only as long) as one or more events pertaining to that flow were
> > scheduled to the port in question, but are not yet released."
> >
> > Maybe it needs to be more meaty, describing what released means. I don't
> > have the full context of the documentation in my head when I'm writing this.
> >
>
> Will take a look to reword a bit
>
> >
> > > + * the scheduler is now allowed to schedule events from that flow from to another port.
> > > + * However, the atomic locks may be still held until the next rte_event_dequeue_burst()
> > > + * call; enqueuing an event with opt type @ref RTE_EVENT_OP_RELEASE allows,
> >
> > Is ";" intended?
> >
> > > + * but does not force, the scheduler to release the atomic locks early.
> >
> > instead of "not force", can use the term _hint_ the driver and reword.
>
> Ok.
> >
> > > + *
> > > + * Early atomic lock release may increase parallelism and thus system
> > >   * 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
> > > + * then this operation type 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*
> > > - * or no scheduling context is held then this function may be an NOOP,
> > > - * depending on the implementation.
> > > + * Events with this operation type must only be enqueued to the same port that the
> > > + * event to be released was dequeued from. The @ref rte_event.impl_opaque
> > > + * field in the release event must have the same value as that in the original dequeued event.
> > >   *
> > > - * This operation must only be enqueued to the same port that the
> > > - * event to be released was dequeued from.
> > > + * If a dequeued event is re-enqueued with operation type of @ref RTE_EVENT_OP_RELEASE,
> > > + * then any subsequent enqueue of that event - or a copy of it - must be done as event of type
> > > + * @ref RTE_EVENT_OP_NEW, not @ref RTE_EVENT_OP_FORWARD. This is because any context for
> > > + * the originally dequeued event, i.e. atomic locks, or reorder buffer entries, will have
> > > + * been removed or invalidated by the release operation.
> > >   */
> > >
> > >  /**
> > > @@ -1517,56 +1524,110 @@ struct rte_event {
> > >                 /** Event attributes for dequeue or enqueue operation */
> > >                 struct {
> > >                         uint32_t flow_id:20;
> > > -                       /**< Targeted flow identifier for the enqueue and
> > > -                        * dequeue operation.
> > > -                        * The value must be in the range of
> > > -                        * [0, nb_event_queue_flows - 1] which
> > > -                        * previously supplied to rte_event_dev_configure().
> > > +                       /**< Target flow identifier for the enqueue and dequeue operation.
> > > +                        *
> > > +                        * For @ref RTE_SCHED_TYPE_ATOMIC, this field is used to identify a
> > > +                        * flow for atomicity within a queue & priority level, such that events
> > > +                        * from each individual flow will only be scheduled to one port at a time.
> > > +                        *
> > > +                        * 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 application or event adapter use,
> > > +                        * and is not considered in scheduling decisions.
> >
> >
> > cnxk driver is considering this for scheduling decision to
> > differentiate the producer i.e event adapters.
> > If other drivers are not then we can change the language around it is
> > implementation defined.
> >
> How does the event type influence the scheduling decision? I can drop the

For cnxk, From HW POV, the flow ID is 32 bit which is divided between
flow_id(20 bit), sub event type(8bit) and
event type(4bit)

> last line here

Yes. Please


 > but it seems strange to me that the type of event could affect
> things. I would have thought that with the eventdev API only the queue,
> flow id, and priority would be factors in scheduling?

>
> >
> > > +                        *
> > >                          * @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 application or event adapter use,
> > > +                        * and is not considered in scheduling decisions.
> >
> >
> > cnxk driver is considering this for scheduling decision to
> > differentiate the producer i.e event adapters.
> > If other drivers are not then we can change the language around it is
> > implementation defined.
> >
> > >                          */
> > >                         uint8_t op:2;
> > > -                       /**< The type of event enqueue operation - new/forward/
> > > -                        * etc.This field is not preserved across an instance
> > > -                        * and is undefined on dequeue.
> > > -                        * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> > > +                       /**< The type of event enqueue operation - new/forward/ etc.
> > > +                        *
> > > +                        * This field is *not* preserved across an instance
> > > +                        * and is implementation dependent on dequeue.
> > > +                        *
> > > +                        * @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.
> >
> > I am worried about some application explicitly start setting this to
> > zero on every enqueue.
> > Instead, can we say application should not touch the field, Since every eventdev
> > operations starts with dequeue() driver can fill to the correct value.
> >
>
> I'll set this to zero on "NEW", or untouched on FORWARD/RELEASE.

OK

> If we don't state that it should be zeroed on NEW or untouched
> otherwise we cannot use the space in future without ABI break.
>
> > > +                        */
> > >                         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 configured.
> > > +                        * For queues where only a single scheduling type is available,
> > > +                        * this field must be set to match the configured scheduling type.
> > > +                        *
> > > +                        * 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().
> > > +                        * The value must be less than @ref rte_event_dev_config.nb_event_queues
> > > +                        * which was previously supplied to rte_event_dev_configure().
> >
> > Some reason, similar text got removed for flow_id. Please add the same.
> >
>
> That was deliberate based on discussion on V2. See:
>
> http://inbox.dpdk.org/dev/Zby3nb4NGs8T5odL@bricha3-MOBL.ger.corp.intel.com/
>
> and wider thread discussion starting here:
>
> http://inbox.dpdk.org/dev/ZbvOtAEpzja0gu7b@bricha3-MOBL.ger.corp.intel.com/
>
> Basically, the comment is wrong based on what the code does now. No event
> adapters or apps are limiting the flow-id, and nothing seems broken, so we
> can remove the comment.

OK

>
  

On Wed, Feb 21, 2024 at 03:01:06PM +0530, Jerin Jacob wrote:
> On Tue, Feb 20, 2024 at 11:09 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote:
> > > On Fri, Feb 2, 2024 at 6:11 PM Bruce Richardson
> > > <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> > > > ---
<snip>
> > > >                         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 application or event adapter use,
> > > > +                        * and is not considered in scheduling decisions.
> > >
> > >
> > > cnxk driver is considering this for scheduling decision to
> > > differentiate the producer i.e event adapters.
> > > If other drivers are not then we can change the language around it is
> > > implementation defined.
> > >
> > How does the event type influence the scheduling decision? I can drop the
> 
> For cnxk, From HW POV, the flow ID is 32 bit which is divided between
> flow_id(20 bit), sub event type(8bit) and
> event type(4bit)
> 
> > last line here
> 
> Yes. Please
> 
> 
Dropping last sentence in v4.