On 7 April 2017 at 07:29, 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) < > [email protected]> >> 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. 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). > > AFAIK, the scalable scheduler conforms to the spec (there could still be > undetected bugs of course). > Kevin, this is the test/common_plat/validation/api/scheduler test? Which > platform and configuration? I haven't seen this failure. > I get the following result on a multicore ARM target: > > $ test/common_plat/validation/api/scheduler/scheduler_main > ... > Run Summary: Type Total Ran Passed Failed Inactive > > suites 1 1 n/a 0 0 > > tests 35 35 35 0 0 > > asserts 2381749 2381749 2381749 0 n/a > > > Elapsed time = 23.919 seconds > >
Looking at the patch carefully, the release is NOT added by this patch. This patch corrects the release to take the sync type into account (as the release APIs are different for different sync types). >> >> 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. > The default queue size is 4096 which seems plenty. We need to check how > many events fill_queues() expects to be able to enqueue. > Agree, this change should not be part of this patch. The change is correct in the sense that 'odp_queue_enq' should not be expected to pass always. Whether it failed in an actual test or not is secondary. >> >> -Petri >> >> >> >> 2017-04-07 16:22 GMT+08:00 Savolainen, Petri (Nokia - FI/Espoo) < > [email protected]<mailto: > [email protected]>>: >> >> >>> -----Original Message----- >>> From: lng-odp [mailto:[email protected]<mailto: > [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:[email protected]>> >>> Reviewed-by: Ola Liljedahl <[email protected]<mailto: > [email protected]>> > I have no recollection of having reviewed this patch... > And I can't find it in gerrit for our local repo/branch. > > >>> --- >>> .../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
