On 17 February 2015 at 15:49, Ola Liljedahl <[email protected]> wrote: > On 17 February 2015 at 11:49, Petri Savolainen > <[email protected]> wrote: >> Improved documentation and definition of atomic and ordered >> queue synchronisation. >> >> Signed-off-by: Petri Savolainen <[email protected]> >> >> --- >> This is the ordered queue definition (in patch format) promised >> in the call yesterday. >> --- >> include/odp/api/queue.h | 21 +++++++++++++++++++-- >> include/odp/api/schedule.h | 15 ++++++++++++--- >> 2 files changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/include/odp/api/queue.h b/include/odp/api/queue.h >> index 9519edf..1d67a3c 100644 >> --- a/include/odp/api/queue.h >> +++ b/include/odp/api/queue.h >> @@ -108,12 +108,29 @@ extern "C" { >> >> /** >> * @def ODP_SCHED_SYNC_ATOMIC >> - * Atomic queue >> + * Atomic queue synchronisation >> + * >> + * Events from an atomic queue can be scheduled only to a single thread at a > I question the use of "schedule to", is this really proper language? > Events are scheduled but the events selected are then 'dispatched' to a > thread. > Suggestion: "Events from an atomic queue can be dispatched only to a > single thread at a time, this avoids ". Should have been: "Events from an atomic queue can be dispatched only to a single thread at a time." The "avoid" stuff is below.
> >> + * time. This quarantees atomic access to the queue context and maintains >> event > Spelling error, should be "guarantees". > Suggestion: "Atomic scheduling provides mutual exclusion and atomic > access to the associated queue context and maintains event ordering. > This enables the user to avoid synchronization and event reordering in > software." > >> + * ordering, which helps the user to avoid SW locking. >> + * >> + * The atomic queue is dedicated to the thread until it requests another >> event > Suggestion: "The selected atomic queue is dedicated to the thread > until the thread requests another event from the scheduler (which > implicitly releases the queue) or the thread calls > odp_schedule_release_atomic() (which allows the scheduler to release > the queue immediately)." > >> + * from the scheduler (implicitly requests to release the current queue) or >> + * calls odp_schedule_release_atomic(), which hints the scheduler to >> release the >> + * queue early. >> */ >> >> /** >> * @def ODP_SCHED_SYNC_ORDERED >> - * Ordered queue >> + * Ordered queue synchronisation >> + * >> + * Events from an ordered queue can be scheduled to multiple threads for >> + * parallel processing. > The phrase "parallel processing" indicates that *one* piece of work is > being processed in parallel by multiple processors. But we are not > talking about one event being processed in parallel, it is different > events (from the same queue) that may be processed concurrently (by > different processors). It's borderline but I suggest we avoid the use > of "parallel" in order to minimize the risk for confusion. > > Suggestion: "Events from an ordered queue can be scheduled and > dispatched to multiple threads for concurrent processing." > >> The original enqueue order of the source queue is > Isn't it the event dequeue order of the source queue that is being > maintained? Yes this might be the same as the enqueue order to the > source queue (since queues supposedly are FIFO) but if enqueueing is > done from another ordered queue, threads may call odp_queue_enq() in a > different order and the events will be reordered. It is this enqueue > order the software sees but it is not this order ordered queues are > maintaining. > > Suggestion: "The (dequeue) event order of the source queue is > maintained when events are enqueued to their destination queue(s) > (before another call to the scheduler)." Maybe drop the parentheses > from the last phrase. > >> + * maintained when events are enqueued to their destination queue(s) before >> + * another schedule call. Events from the same (source) queue appear in >> their >> + * original order when dequeued from a destination queue. The destination >> + * queue type (POLL/SCHED) or synchronisation (NONE/ATOMIC/ORDERED) is not >> + * limited. > What about packet input and packet output queues? > > Suggestion: "The destination queue type (polled, scheduled or packet > input/output) or synchronization model (none, ordered or atomic) is of > no concern." > >> */ >> >> /** >> diff --git a/include/odp/api/schedule.h b/include/odp/api/schedule.h >> index 5c08357..378ca19 100644 >> --- a/include/odp/api/schedule.h >> +++ b/include/odp/api/schedule.h >> @@ -93,9 +93,9 @@ int odp_schedule_multi(odp_queue_t *from, uint64_t wait, >> odp_event_t events[], >> * Pause scheduling >> * >> * Pause global scheduling for this thread. After this call, all schedule >> calls >> - * will return only locally reserved buffers (if any). User can exit the >> + * will return only locally reserved events (if any). User can exit the > "reserved"? > Reserved items can normally be unreserved but I don't think this is > the case here. > Better to use "pre-scheduled events" which must be consumed by > (dispatched to) the thread in question. > >> * schedule loop only after the schedule function indicates that there's no >> more >> - * buffers (no more locally reserved buffers). >> + * events (no more locally reserved events). >> * >> * Must be used with odp_schedule() and odp_schedule_multi() before exiting >> (or >> * stalling) the schedule loop. >> @@ -111,7 +111,16 @@ void odp_schedule_pause(void); >> void odp_schedule_resume(void); >> >> /** >> - * Release currently hold atomic context >> + * Release the current atomic context >> + * >> + * This call is valid only for source queues with atomic synchronisation. It > English or American spelling? "s" vs. "z". > >> + * hints the scheduler that the user has completed processing of the >> critical >> + * section (which depends on the atomic synchronisation). The scheduler is >> now > Same here. > >> + * allowed to schedule another event(s) from the same queue to another >> thread. > Suggestion: "The scheduler is now allowed to schedule and dispatch > further events from the same queue to some other thread". > >> + * >> + * Early atomic context release may increase parallelism and thus system >> + * performance, but user needs to design carefully the split into critical >> vs. > "design carefully" => "carefully design" > >> + * non-critical sections. >> */ >> void odp_schedule_release_atomic(void); >> >> -- >> 2.3.0 >> >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
