On Fri, Jul 31, 2015 at 6:52 AM, Bala Manoharan <[email protected]> wrote:
> > > On 31 July 2015 at 17:48, Bill Fischofer <[email protected]> > wrote: > >> >> >> On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan < >> [email protected]> wrote: >> >>> Hi, >>> >>> Comments inline... >>> >>> On 31 July 2015 at 08:11, Bill Fischofer <[email protected]> >>> wrote: >>> >>>> Signed-off-by: Bill Fischofer <[email protected]> >>>> --- >>>> include/odp/api/schedule.h | 38 ++++++++++++++++++++++++-------------- >>>> 1 file changed, 24 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/include/odp/api/schedule.h b/include/odp/api/schedule.h >>>> index 95fc8df..0ab91e4 100644 >>>> --- a/include/odp/api/schedule.h >>>> +++ b/include/odp/api/schedule.h >>>> @@ -147,21 +147,31 @@ void odp_schedule_resume(void); >>>> void odp_schedule_release_atomic(void); >>>> >>>> /** >>>> - * Release the current ordered context >>>> - * >>>> - * This call is valid only for source queues with ordered >>>> synchronization. It >>>> - * hints the scheduler that the user has done all enqueues that need >>>> to maintain >>>> - * event order in the current ordered context. The scheduler is >>>> allowed to >>>> - * release the ordered context of this thread and avoid reordering any >>>> following >>>> - * enqueues. However, the context may be still held until the next >>>> - * odp_schedule() or odp_schedule_multi() call - this call allows but >>>> does not >>>> - * force the scheduler to release the context early. >>>> - * >>>> - * Early ordered context release may increase parallelism and thus >>>> system >>>> - * performance, since scheduler may start reordering events sooner >>>> than the next >>>> - * schedule call. >>>> + * Release the order associated with an event >>>> + * >>>> + * This call tells the scheduler that order no longer needs to be >>>> maintained >>>> + * for the specified event. This call is needed if, for example, the >>>> caller >>>> >>> >>> [Bala] This release ordered is for the specified event in this ordered >>> context only. coz it is always possible >>> that this event may get enqueued into other ordered/Atomic queue and the >>> ordering should be maintained >>> in that context. >>> >> >> Not sure I understand your point here. Ordering is something that is >> inherited from a source queue and each queue is independent. If you pass >> events through a series of ordered queues then you get end-to-end ordering, >> however if you release an event and then add it to another ordered queue it >> will maintain order from that point however might be reordered with respect >> to the original queue from which it was released. >> > > I agree with your description above. My point was that the new description > says > > *" This call tells the scheduler that order no longer needs to be > maintained for the specified event" * > but the interpretation means that the order will not be maintained for the > specified event hereafter. But actually what this call does is it removes > ordering for this event in this specific ordered queue (ordered context ) > as you explained above the ordering will be maintained by the scheduler for > this specified event when it gets enqueued into another ordered/ atomic > queue. so IMO the description should be* " This call tells the scheduler > that order no longer needs to be maintained for the specified event in the > current ordered context" * > > OK, I agree. I'll expand the documentation to make that point clear that the ordering is released only for the current context. > >> >>> >>> + * will free or otherwise dispose of an event that came from an ordered >>>> queue >>>> + * without enqueuing it to another queue. This call does not effect the >>>> >>> >>> [Bala] The use-case of freeing or disposing the event can be handled >>> implicitly by the implementation >>> since freeing should be done by calling odp_event_free() API and the >>> implementation is free to release >>> ordered context during free. >>> >> >> I thought of that, however that would add a lot of unnecessary checking >> overhead to the free path, which is itself a performance path. If we're >> going to provide the ability to release order then I don't think it's >> unreasonable to ask the application to use that in a disciplined manner. >> > > Yes. But lets say if an application does odp_event_free() without calling > this call then the implementation will have to release the ordering for > that event. coz this specific case will cause a queue halt if application > calls odp_event_free() without calling release ordered() API. > > No different than any number of other possible programming errors on the part of the application (e.g., leaks memory, tries to double-free, forgets to release a lock, etc.). Why should this one be special? I think we just need to make it clear that if the application wishes to use ordered queues that it needs to use them correctly, and explain how to use this API properly in that context. To the point Alex keeps rightly bringing up, we need to consider this in terms of things like fragmentation and reassembly. With this definition odp_schedule_release_ordered() permits proper fragment reassembly since each fragment can be released from ordering as it enters the reassembly buffer and then when the final piece arrives the reassembled packet can continue using the final fragment's sequence number. We seem to need a corresponding odp_schedule_insert_ordered() call to cover the case of one packet in (with a sequence number) that needs to be split into multiple parts that are correctly sequenced. I need to think a bit more about the syntax and semantics of such a capability. Is this something that either Cavium or Freescale currently support in your HW? That would be a good basis for constructing the semantics (and restrictions) surrounding such an API. > Regards, > Bala > >> >> >>> >>> >>>> + * ordering associated with any other event held by the caller. >>>> + * >>>> + * Order release may increase parallelism and thus system performance, >>>> since >>>> + * the scheduler may start resolving reordered events sooner than the >>>> next >>>> + * odp_queue_enq() call. >>>> + * >>>> + * @param ev The event to be released from order preservation. >>>> + * >>>> + * @retval 0 Success. Upon return ev behaves as if it originated >>>> + * from a parallel rather than an ordered queue. >>>> + * >>>> + * @retval <0 Failure. This can occur if the event did not >>>> originate >>>> + * from an ordered queue (caller error) or the >>>> implementation >>>> + * is unable to release order at this time. In this >>>> case, >>>> + * the caller must not dispose of ev without enqueing it >>>> + * first to avoid deadlocking other events originating >>>> from >>>> + * ev's ordered queue. >>>> */ >>>> -void odp_schedule_release_ordered(void); >>>> +int odp_schedule_release_ordered(odp_event_t ev); >>>> >>>> /** >>>> * Prefetch events for next schedule call >>>> >>> >>> Regards, >>> Bala >>> >>>> -- >>>> 2.1.4 >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> [email protected] >>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>> >>> >> >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
