On Sat, Apr 8, 2017 at 4:31 PM, Ola Liljedahl <ola.liljed...@linaro.org> wrote: > On 7 April 2017 at 14:06, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolai...@nokia-bell-labs.com> wrote: >> >> >> From: Kevin Wang [mailto:kevin.w...@linaro.org] >> Sent: Friday, April 07, 2017 11:45 AM >> To: Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolai...@nokia-bell-labs.com> >> Cc: Kevin Wang <kevin.w...@arm.com>; lng-odp@lists.linaro.org >> 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) >> <petri.savolai...@nokia-bell-labs.com<mailto:petri.savolai...@nokia-bell-labs.com>>: >> >> >>> -----Original Message----- >>> From: lng-odp >>> [mailto:lng-odp-boun...@lists.linaro.org<mailto:lng-odp-boun...@lists.linaro.org>] >>> On Behalf Of Kevin >>> Wang >>> Sent: Friday, April 07, 2017 11:07 AM >>> To: lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> >>> Cc: Kevin Wang <kevin.w...@arm.com<mailto:kevin.w...@arm.com>> >>> 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 <kevin.w...@arm.com<mailto:kevin.w...@arm.com>> >>> Reviewed-by: Ola Liljedahl >>> <ola.liljed...@arm.com<mailto:ola.liljed...@arm.com>> >>> --- >>> .../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
