So seems a new API odp_schedule_release_context() is required. So I'll redirect the patch to api-next. Should it be an internal function?
2017-04-09 9:35 GMT+08:00 Bill Fischofer <[email protected]>: > On Sat, Apr 8, 2017 at 4:31 PM, Ola Liljedahl <[email protected]> > wrote: > > On 7 April 2017 at 14:06, Savolainen, Petri (Nokia - FI/Espoo) > > <[email protected]> wrote: > >> > >> > >> From: Kevin Wang [mailto:[email protected]] > >> Sent: Friday, April 07, 2017 11:45 AM > >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- > labs.com> > >> Cc: Kevin Wang <[email protected]>; [email protected] > >> Subject: Re: [lng-odp] [PATCH] validation: scheduler: Release context > before the end of the scheduler test > >> > >> 1.Release context is just to be added for scalable scheduler in > scheduler_test_groups(). I think it does no harms to other scheduler here. > >> 2.This code is to be removed for the scalable scheduler, We use ring > buffer to implement the queue. So it is possible the enqueue operation > failed if the ring buffer is full. > >> > >> Kevin > >> > >> > >> 1. Validation tests are written against API spec. The spec says that > context release is a hint. > > Actually this is what the spec says: > > /** > > * Release the current atomic context > > * > > * This call is valid only for source queues with atomic > synchronization. It > > > > /** > > * Release the current ordered context > > * > > * This call is valid only for source queues with ordered > synchronization. It > > > > So I think the validation test is violating the spec by calling > > odp_schedule_release_ordered() also for atomic/parallel queue/synch > > types. > > odp_schedule_sync_t sync[] = {ODP_SCHED_SYNC_PARALLEL, > > ODP_SCHED_SYNC_ATOMIC, > > ODP_SCHED_SYNC_ORDERED}; > > for (i = 0; i < 3; i++) { > > qp.sched.sync = sync[i]; > > > > The release calls are hints in the sense that the ODP implementation > > must not necessarily release the atomic or ordered context, this > > release ca be delayed until the scheduler is invoked again. > > > > Originally the scalable scheduler reported an error when calling > > odp_schedule_release_atomic() (odp_schedule_release_ordered()) when an > > atomic (ordered) queue was *not* being processed and Kevin's patch was > > needed for the scheduler validation test to pass. Later we relaxed the > > behaviour when an invalid call is made, just silently ignoring invalid > > calls. > > > > IMO Kevin's patch changes the scheduler validation test to follow the > > spec. We should change either the validation test or the spec. > > I agree, but I think a better solution is to deprecate both and > replace them with an odp_schedule_release_context() API that hints > that the caller is done with any active synchronization context being > provided by the scheduler. This call would explicitly be a no-op if no > such context is active. > > > > >> Your scheduler must not depend on extra context release calls, but must > apply to the spec. A validation test written against the spec must work. > May be the application is not working by the spec. But adding a context > release hint, does not fix that (== guarantee that context is actually > released). > > The spec (currently) doesn't say anything about calling the wrong > > release call. At best this is undefined behaviour, at worst illegal > > behaviour. > > > >> > >> 2. This patch is about context releases. This fixes an enqueue issue => > should not be in this patch. How big queue capacity the test expects ? If > it’s reasonable, maybe you should increase ring size instead. Soon we’ll > have queue size param and tests can be updated to check that. In the > meanwhile it feels wrong to remove error checks from validation suite. > >> > >> -Petri > >> > >> > >> > >> 2017-04-07 16:22 GMT+08:00 Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]<mailto:petri. > [email protected]>>: > >> > >> > >>> -----Original Message----- > >>> From: lng-odp [mailto:[email protected]<mailto:lng-odp- > [email protected]>] On Behalf Of Kevin > >>> Wang > >>> Sent: Friday, April 07, 2017 11:07 AM > >>> To: [email protected]<mailto:[email protected]> > >>> Cc: Kevin Wang <[email protected]<mailto:[email protected]>> > >>> Subject: [lng-odp] [PATCH] validation: scheduler: Release context > before > >>> the end of the scheduler test > >>> > >>> If the scheduler sync type is atomic or ordered, > >>> need to release the context. > >> > >> Release context is actually a hint. It does not guarantee that context > is released. Application needs to call schedule() and receive > _EVENT_INVALID to be sure that it does not hold a context anymore. > >> > >> > >>> > >>> Signed-off-by: Kevin Wang <[email protected]<mailto:kev > [email protected]>> > >>> Reviewed-by: Ola Liljedahl <[email protected]<mailto: > [email protected]>> > >>> --- > >>> .../common_plat/validation/api/scheduler/scheduler.c | 20 > +++++++++++---- > >>> ----- > >>> 1 file changed, 11 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/test/common_plat/validation/api/scheduler/scheduler.c > >>> b/test/common_plat/validation/api/scheduler/scheduler.c > >>> index 952561c..2631001 100644 > >>> --- a/test/common_plat/validation/api/scheduler/scheduler.c > >>> +++ b/test/common_plat/validation/api/scheduler/scheduler.c > >>> @@ -129,6 +129,14 @@ static int exit_schedule_loop(void) > >>> return ret; > >>> } > >>> > >>> +static void release_context(odp_schedule_sync_t sync) > >>> +{ > >>> + if (sync == ODP_SCHED_SYNC_ATOMIC) > >>> + odp_schedule_release_atomic(); > >>> + else if (sync == ODP_SCHED_SYNC_ORDERED) > >>> + odp_schedule_release_ordered(); > >>> +} > >>> + > >>> void scheduler_test_wait_time(void) > >>> { > >>> int i; > >>> @@ -251,8 +259,7 @@ void scheduler_test_queue_destroy(void) > >>> CU_ASSERT_FATAL(u32[0] == MAGIC); > >>> > >>> odp_buffer_free(buf); > >>> - odp_schedule_release_ordered(); > >>> - > >>> + release_context(qp.sched.sync); > >>> CU_ASSERT_FATAL(odp_queue_destroy(queue) == 0); > >>> } > >>> > >>> @@ -478,6 +485,7 @@ void scheduler_test_groups(void) > >>> odp_schedule_group_leave(mygrp1, &mymask); > >>> odp_schedule_group_leave(mygrp2, &mymask); > >>> > >>> + release_context(qp.sched.sync); > >> > >> Why this hint was added? Maybe a proper pause()/schedule() sequence is > actually missing here ? > >> > >> > >> > >>> /* Done with queues for this round */ > >>> CU_ASSERT_FATAL(odp_queue_destroy(queue_grp1) == 0); > >>> CU_ASSERT_FATAL(odp_queue_destroy(queue_grp2) == 0); > >>> @@ -820,12 +828,7 @@ static int schedule_common_(void *arg) > >>> } > >>> } > >>> > >>> - if (sync == ODP_SCHED_SYNC_ATOMIC) > >>> - odp_schedule_release_atomic(); > >>> - > >>> - if (sync == ODP_SCHED_SYNC_ORDERED) > >>> - odp_schedule_release_ordered(); > >>> - > >>> + release_context(sync); > >>> odp_ticketlock_lock(&globals->lock); > >>> > >>> globals->buf_count -= num; > >>> @@ -959,7 +962,6 @@ static void fill_queues(thread_args_t *args) > >>> } > >>> > >>> ret = odp_queue_enq(queue, ev); > >>> - CU_ASSERT_FATAL(ret == 0); > >> > >> > >> Why this line was removed? Is it bug in the test? Did you really meant > to remove the line? It does not belong under the subject line of this patch. > >> > >> -Petri > >> > >> > >>> > >>> if (ret) > >>> > >>> odp_buffer_free(buf); > >>> -- > >>> 2.7.4 > >> > >> > >> > >> -- > >> Thanks, > >> Kevin > -- Thanks, Kevin
