| Message ID | 1438310507-10750-6-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, Comments inline... On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > 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. + * 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. > + * 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 > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > Hi, > > Comments inline... > > On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> 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. > > + * 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. > > >> + * 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 >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
On 31 July 2015 at 17:48, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan <bala.manoharan@linaro.org > > wrote: > >> Hi, >> >> Comments inline... >> >> On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> --- >>> 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" * > >> >> + * 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. 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 >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >
On Fri, Jul 31, 2015 at 6:52 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > > > On 31 July 2015 at 17:48, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> >> >> On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan < >> bala.manoharan@linaro.org> wrote: >> >>> Hi, >>> >>> Comments inline... >>> >>> On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>> --- >>>> 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 >>>> lng-odp@lists.linaro.org >>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>> >>> >> >
On 31 July 2015 at 20:03, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Fri, Jul 31, 2015 at 6:52 AM, Bala Manoharan <bala.manoharan@linaro.org > > wrote: > >> >> >> On 31 July 2015 at 17:48, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> >>> >>> On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan < >>> bala.manoharan@linaro.org> wrote: >>> >>>> Hi, >>>> >>>> Comments inline... >>>> >>>> On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>> >>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>> --- >>>>> 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. > But this release ordered context is different because ODP expectation is that the ordered context is released implicitly incase of odp_schedule() call so extending from logic in seems reasonable to release the ordered context during odp_event_free() > 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 >>>>> lng-odp@lists.linaro.org >>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>>> >>>> >>>> >>> >> >
On Fri, Jul 31, 2015 at 8:17 AM, Bala Manoharan <bala.manoharan@linaro.org> wrote: > > > On 31 July 2015 at 20:03, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> >> >> On Fri, Jul 31, 2015 at 6:52 AM, Bala Manoharan < >> bala.manoharan@linaro.org> wrote: >> >>> >>> >>> On 31 July 2015 at 17:48, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>>> >>>> >>>> On Fri, Jul 31, 2015 at 1:38 AM, Bala Manoharan < >>>> bala.manoharan@linaro.org> wrote: >>>> >>>>> Hi, >>>>> >>>>> Comments inline... >>>>> >>>>> On 31 July 2015 at 08:11, Bill Fischofer <bill.fischofer@linaro.org> >>>>> wrote: >>>>> >>>>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>>>> --- >>>>>> 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. >> > > But this release ordered context is different because ODP expectation is > that the ordered context is released implicitly incase of odp_schedule() > call so extending from logic in seems reasonable to release the ordered > context during odp_event_free() > > That's an interesting perspective. My understanding is that order is released as part of a subsequent odp_queue_enq() operation, because that is what can unblock other waiters in the source queue's reorder queue. So if the recipient of an event from an ordered queue does anything other than enq it to some other queue, then it needs to call odp_schedule_release_ordered() to prevent blockage. Remember, ordered queues behave more like parallel queues than like atomic queues in this regard. Atomic queues are not scalable since by definition they serialize processing. On the fragmentation question I raised earlier, I believe that since order is associated with a source queue and if an application is itself fragmenting a packet then since those fragments did not originate on a source queue there's no need for an insert() API. Instead, if the fragments are to be part of the ordered context of the base packet then they should all be enqueued to a downstream queue via odp_queue_enq_multi(), which needs to be enhanced to cover this case. Alex: Would that be a workable solution for you? > >> 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 >>>>>> lng-odp@lists.linaro.org >>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>> >>>>> >>>> >>> >> >
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 + * 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 + * 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
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- include/odp/api/schedule.h | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)