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. 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). 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
