From: Bruce Richardson <bruce.richardson@intel.com>
To: "Mattias Rönnblom" <hofors@lysator.liu.se>
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>
Subject: Re: [PATCH v2 10/11] eventdev: RFC clarify comments on scheduling types
Date: Wed, 24 Jan 2024 11:21:16 +0000 [thread overview]
Message-ID: <ZbDyrKtt_2zl4c1p@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <cabd7136-fdf4-4cc9-9672-2f025f609d1d@lysator.liu.se>
On Tue, Jan 23, 2024 at 05:19:18PM +0100, Mattias Rönnblom wrote:
> On 2024-01-19 18:43, Bruce Richardson wrote:
> > The description of ordered and atomic scheduling given in the eventdev
> > doxygen documentation was not always clear. Try and simplify this so
> > that it is clearer for the end-user of the application
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >
> > NOTE TO REVIEWERS:
> > I've updated this based on my understanding of what these scheduling
> > types are meant to do. It matches my understanding of the support
> > offered by our Intel DLB2 driver, as well as the SW eventdev, and I
> > believe the DSW eventdev too. If it does not match the behaviour of
> > other eventdevs, let's have a discussion to see if we can reach a good
> > definition of the behaviour that is common.
> > ---
> > lib/eventdev/rte_eventdev.h | 47 ++++++++++++++++++++-----------------
> > 1 file changed, 25 insertions(+), 22 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> > index 2c6576e921..cb13602ffb 100644
> > --- a/lib/eventdev/rte_eventdev.h
> > +++ b/lib/eventdev/rte_eventdev.h
> > @@ -1313,26 +1313,24 @@ struct rte_event_vector {
> > #define RTE_SCHED_TYPE_ORDERED 0
> > /**< Ordered scheduling
> > *
> > - * Events from an ordered flow of an event queue can be scheduled to multiple
> > + * Events from an ordered event queue can be scheduled to multiple
>
> What is the rationale for this change?
>
> An implementation that impose a total order on all events on a particular
> ordered queue will still adhere to the current, more relaxed, per-flow
> ordering semantics.
>
> An application wanting a total order would just set the flow id to 0 on all
> events destined that queue, and it would work on all event devices.
>
> Why don't you just put a note in the DLB driver saying "btw it's total
> order", so any application where per-flow ordering is crucial for
> performance (i.e., where the potentially needless head-of-line blocking is
> an issue) can use multiple queues when running with the DLB.
>
> In the API as-written, the app is free to express more relaxed ordering
> requirements (i.e., to have multiple flows) and it's up to the event device
> to figure out if it's in a position where it can translate this to lower
> latency.
>
Yes, you are right. I'll roll-back or rework this change in V3. Keep it
documented that flow-ordering is guaranteed, but note that some
implementations may use total ordering to achieve that.
> > * ports for concurrent processing while maintaining the original event order.
>
> Maybe it's worth mentioning what is the original event order. "(i.e., the
> order in which the events were enqueued to the queue)". Especially since one
> like to specify what ordering guarantees one have of events enqueued to the
> same queue on different ports and by different lcores).
>
> I don't know where that information should go though, since it's relevant
> for both atomic and ordered-type queues.
>
It's probably more relevant for ordered, but I'll try and see where it's
best to go.
> > * This scheme enables the user to achieve high single flow throughput by
> > - * avoiding SW synchronization for ordering between ports which bound to cores.
> > - *
> > - * The source flow ordering from an event queue is maintained when events are
> > - * enqueued to their destination queue within the same ordered flow context.
> > - * An event port holds the context until application call
> > - * rte_event_dequeue_burst() from the same port, which implicitly releases
> > - * the context.
> > - * User may allow the scheduler to release the context earlier than that
> > - * by invoking rte_event_enqueue_burst() with RTE_EVENT_OP_RELEASE operation.
> > - *
> > - * Events from the source queue appear in their original order when dequeued
> > - * from a destination queue.
> > - * Event ordering is based on the received event(s), but also other
> > - * (newly allocated or stored) events are ordered when enqueued within the same
> > - * ordered context. Events not enqueued (e.g. released or stored) within the
> > - * context are considered missing from reordering and are skipped at this time
> > - * (but can be ordered again within another context).
> > + * avoiding SW synchronization for ordering between ports which are polled by
> > + * different cores.
> > + *
> > + * As events are scheduled to ports/cores, the original event order from the
> > + * source event queue is recorded internally in the scheduler. As events are
> > + * returned (via FORWARD type enqueue) to the scheduler, the original event
> > + * order is restored before the events are enqueued into their new destination
> > + * queue.
>
> Delete the first sentence on implementation.
>
> "As events are re-enqueued to the next queue (with the op field set to
> RTE_EVENT_OP_FORWARD), the event device restores the original event order
> before the events arrive on the destination queue."
>
> > + *
> > + * Any events not forwarded, ie. dropped explicitly via RELEASE or implicitly
> > + * released by the next dequeue from a port, are skipped by the reordering
> > + * stage and do not affect the reordering of returned events.
> > + *
> > + * The ordering behaviour of NEW events with respect to FORWARD events is
> > + * undefined and implementation dependent.
>
> For some reason I find this a little vague. "NEW and FORWARD events enqueued
> to a queue are not ordered in relation to each other (even if the flow id is
> the same)."
>
> I think I agree that NEW shouldn't be ordered vis-a-vi FORWARD, but maybe
> one should say that an event device should avoid excessive reordering NEW
> and FORWARD events.
>
> I think it would also be helpful to address port-to-port ordering guarantees
> (or a lack thereof).
>
> "Events enqueued on one port are not ordered in relation to events enqueued
> on some other port."
>
> Or are they? Not in DSW, at least, and I'm not sure I see a use case for
> such a guarantee, but it's a little counter-intuitive to have them
> potentially re-shuffled.
>
> (This is also relevant for atomic queues.)
>
Ack.
> > *
> > * @see rte_event_queue_setup(), rte_event_dequeue_burst(), RTE_EVENT_OP_RELEASE
> > */
> > @@ -1340,18 +1338,23 @@ struct rte_event_vector {
> > #define RTE_SCHED_TYPE_ATOMIC 1
> > /**< Atomic scheduling
> > *
> > - * Events from an atomic flow of an event queue can be scheduled only to a
> > + * Events from an atomic flow, identified by @ref rte_event.flow_id,
>
> A flow is identified by the combination of queue_id and flow_id, so if you
> reference one you should also reference the other.
>
Yes, this is probably one to be reflected globally. Also on your previous
comment about priority, I believe that a flow for ordering guarantees
should be a combination of queue_id, flow_id and priority. Two packets with
different priorities should expect to be reordered, since that tends to be
what priority implies.
> > + * of an event queue can be scheduled only to a
> > * single port at a time. The port is guaranteed to have exclusive (atomic)
> > * access to the associated flow context, which enables the user to avoid SW
> > * synchronization. Atomic flows also help to maintain event ordering
>
> "help" here needs to go, I think. It sounds like a best-effort affair. The
> atomic queue ordering guarantees (or the lack thereof) should be spelled
> out.
>
> "Event order in an atomic flow is maintained."
Ack.
>
> > - * since only one port at a time can process events from a flow of an
> > + * since only one port at a time can process events from each flow of an
> > * event queue.
>
> Yes, and *but also since* the event device is not reshuffling events
> enqueued to an atomic queue. And that's more complicated than just something
> that falls out of atomicity, especially if you assume that FORWARD type
> enqueues are not ordered with other FORWARD type enqueues on a different
> port.
>
Ack.
> > *
> > - * The atomic queue synchronization context is dedicated to the port until
> > + * The atomic queue synchronization context for a flow is dedicated to the port until
>
> What is an "atomic queue synchronization context" (except for something that
> makes for long sentences).
>
Yes, it's rather wordy. I like the idea of using lock terminology you
suggest. The use of the word "contexts" in relation to atomic/ordered I
find confusing myself too.
> How about:
> "The atomic flow is locked to the port until /../"
>
> You could also used the word "bound" instead of "locked".
>
> > * application call rte_event_dequeue_burst() from the same port,
> > * which implicitly releases the context. User may allow the scheduler to
> > * release the context earlier than that by invoking rte_event_enqueue_burst()
> > - * with RTE_EVENT_OP_RELEASE operation.
> > + * with RTE_EVENT_OP_RELEASE operation for each event from that flow. The context
> > + * is only released once the last event from the flow, outstanding on the port,
> > + * is released. So long as there is one event from an atomic flow scheduled to
> > + * a port/core (including any events in the port's dequeue queue, not yet read
> > + * by the application), that port will hold the synchronization context.
>
> In case you like the "atomic flow locked/bound to port", this part would
> also need updating.
>
> Maybe here is a good place to add a note on memory ordering and event
> ordering.
>
> "Any memory stores done as a part of event processing will be globally
> visible before the next event in the same atomic flow is dequeued on a
> different lcore."
>
> I.e., enqueue includes write barrier before the event can be seen.
>
> One should probably mentioned a rmb in dequeue as well.
>
Do we think that that is necessary? I can add it, but I would have thought
that - as with rings - it could be assumed.
/Bruce
next prev parent reply other threads:[~2024-01-24 11:21 UTC|newest]
Thread overview: 123+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-18 13:45 [PATCH v1 0/7] improve eventdev API specification/documentation Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 1/7] eventdev: improve doxygen introduction text Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 2/7] eventdev: move text on driver internals to proper section Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 3/7] eventdev: update documentation on device capability flags Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 4/7] eventdev: cleanup doxygen comments on info structure Bruce Richardson
2024-01-18 13:49 ` Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 5/7] eventdev: improve function documentation for query fns Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 6/7] eventdev: improve doxygen comments on configure struct Bruce Richardson
2024-01-18 13:45 ` [PATCH v1 7/7] eventdev: fix documentation for counting single-link ports Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 00/11] improve eventdev API specification/documentation Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 01/11] eventdev: improve doxygen introduction text Bruce Richardson
2024-01-23 8:57 ` Mattias Rönnblom
2024-01-23 9:06 ` Bruce Richardson
2024-01-24 11:37 ` Mattias Rönnblom
2024-01-31 13:45 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 02/11] eventdev: move text on driver internals to proper section Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 03/11] eventdev: update documentation on device capability flags Bruce Richardson
2024-01-23 9:18 ` Mattias Rönnblom
2024-01-23 9:34 ` Bruce Richardson
2024-01-31 14:09 ` Bruce Richardson
2024-02-02 8:58 ` Mattias Rönnblom
2024-02-02 11:20 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 04/11] eventdev: cleanup doxygen comments on info structure Bruce Richardson
2024-01-23 9:35 ` Mattias Rönnblom
2024-01-23 9:43 ` Bruce Richardson
2024-01-24 11:51 ` Mattias Rönnblom
2024-01-31 14:37 ` Bruce Richardson
2024-02-02 9:24 ` Mattias Rönnblom
2024-02-02 10:30 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 05/11] eventdev: improve function documentation for query fns Bruce Richardson
2024-01-23 9:40 ` Mattias Rönnblom
2024-01-19 17:43 ` [PATCH v2 06/11] eventdev: improve doxygen comments on configure struct Bruce Richardson
2024-01-23 9:46 ` Mattias Rönnblom
2024-01-31 16:15 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 07/11] eventdev: fix documentation for counting single-link ports Bruce Richardson
2024-01-23 9:48 ` Mattias Rönnblom
2024-01-23 9:56 ` Bruce Richardson
2024-01-31 16:18 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 08/11] eventdev: improve doxygen comments on config fns Bruce Richardson
2024-01-23 10:00 ` Mattias Rönnblom
2024-01-23 10:07 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 09/11] eventdev: improve doxygen comments for control APIs Bruce Richardson
2024-01-23 10:10 ` Mattias Rönnblom
2024-01-19 17:43 ` [PATCH v2 10/11] eventdev: RFC clarify comments on scheduling types Bruce Richardson
2024-01-23 16:19 ` Mattias Rönnblom
2024-01-24 11:21 ` Bruce Richardson [this message]
2024-01-31 17:54 ` Bruce Richardson
2024-01-19 17:43 ` [PATCH v2 11/11] eventdev: RFC clarify docs on event object fields Bruce Richardson
2024-01-24 11:34 ` Mattias Rönnblom
2024-02-01 16:59 ` Bruce Richardson
2024-02-02 9:38 ` Mattias Rönnblom
2024-02-02 11:33 ` Bruce Richardson
2024-02-02 12:02 ` Bruce Richardson
2024-02-01 17:02 ` Bruce Richardson
2024-02-02 9:14 ` Bruce Richardson
2024-02-02 9:22 ` Jerin Jacob
2024-02-02 9:36 ` Bruce Richardson
2024-02-02 9:45 ` Mattias Rönnblom
2024-02-02 10:32 ` Bruce Richardson
2024-02-01 9:35 ` Bruce Richardson
2024-02-01 15:00 ` Jerin Jacob
2024-02-01 15:24 ` Bruce Richardson
2024-02-01 16:20 ` Jerin Jacob
2024-02-02 12:39 ` [PATCH v3 00/11] improve eventdev API specification/documentation Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 01/11] eventdev: improve doxygen introduction text Bruce Richardson
2024-02-07 10:14 ` Jerin Jacob
2024-02-08 9:50 ` Mattias Rönnblom
2024-02-09 8:43 ` Jerin Jacob
2024-02-10 7:24 ` Mattias Rönnblom
2024-02-20 16:28 ` Bruce Richardson
2024-02-20 16:33 ` Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 02/11] eventdev: move text on driver internals to proper section Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 03/11] eventdev: update documentation on device capability flags Bruce Richardson
2024-02-07 10:30 ` Jerin Jacob
2024-02-20 16:42 ` Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 04/11] eventdev: cleanup doxygen comments on info structure Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 05/11] eventdev: improve function documentation for query fns Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 06/11] eventdev: improve doxygen comments on configure struct Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 07/11] eventdev: improve doxygen comments on config fns Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 08/11] eventdev: improve doxygen comments for control APIs Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 09/11] eventdev: improve comments on scheduling types Bruce Richardson
2024-02-08 9:18 ` Jerin Jacob
2024-02-08 10:04 ` Mattias Rönnblom
2024-02-20 17:23 ` Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 10/11] eventdev: clarify docs on event object fields and op types Bruce Richardson
2024-02-09 9:14 ` Jerin Jacob
2024-02-20 17:39 ` Bruce Richardson
2024-02-21 9:31 ` Jerin Jacob
2024-02-21 10:28 ` Bruce Richardson
2024-02-20 17:50 ` Bruce Richardson
2024-02-20 18:03 ` Bruce Richardson
2024-02-02 12:39 ` [PATCH v3 11/11] eventdev: drop comment for anon union from doxygen Bruce Richardson
2024-02-21 10:32 ` [PATCH v4 00/12] improve eventdev API specification/documentation Bruce Richardson
2024-02-21 10:32 ` [PATCH v4 01/12] eventdev: improve doxygen introduction text Bruce Richardson
2024-02-26 4:51 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-26 9:59 ` Bruce Richardson
2024-02-29 16:13 ` Jerin Jacob
2024-02-21 10:32 ` [PATCH v4 02/12] eventdev: move text on driver internals to proper section Bruce Richardson
2024-02-26 5:01 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 03/12] eventdev: update documentation on device capability flags Bruce Richardson
2024-02-26 5:07 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 04/12] eventdev: cleanup doxygen comments on info structure Bruce Richardson
2024-02-26 5:18 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 05/12] eventdev: improve function documentation for query fns Bruce Richardson
2024-02-26 5:18 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 06/12] eventdev: improve doxygen comments on configure struct Bruce Richardson
2024-02-26 6:36 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 07/12] eventdev: improve doxygen comments on config fns Bruce Richardson
2024-02-26 6:43 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-26 6:44 ` Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 08/12] eventdev: improve doxygen comments for control APIs Bruce Richardson
2024-02-26 6:44 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 09/12] eventdev: improve comments on scheduling types Bruce Richardson
2024-02-26 6:49 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 10/12] eventdev: clarify docs on event object fields and op types Bruce Richardson
2024-02-26 6:52 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 11/12] eventdev: drop comment for anon union from doxygen Bruce Richardson
2024-02-26 6:52 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-02-21 10:32 ` [PATCH v4 12/12] eventdev: fix doxygen processing of event vector struct Bruce Richardson
2024-02-26 6:53 ` [EXT] " Pavan Nikhilesh Bhagavatula
2024-03-04 15:35 ` Thomas Monjalon
2024-03-04 15:49 ` Bruce Richardson
2024-02-23 12:36 ` [PATCH v4 00/12] improve eventdev API specification/documentation Jerin Jacob
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZbDyrKtt_2zl4c1p@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=abdullah.sevincer@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=hofors@lysator.liu.se \
--cc=jerinj@marvell.com \
--cc=mattias.ronnblom@ericsson.com \
--cc=pbhagavatula@marvell.com \
--cc=pravin.pathak@intel.com \
--cc=sachin.saxena@oss.nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).