The enq failure assert check is already in another patch http://patches.opendataplane.org/patch/8499/. Just to be redundant here. We can ignore it. Further comments should be placed in that patch then.
For the atomic/ordered release, if you see the current code in the upstream, there is already two places call the release function in scheduler.c. I just extend it to another fucntion-scheduler_test_groups() to fix the bugs for scalable scheduler.Ola, If you see the ticket #52637 in our Gerrit, you'll find the detail for the code review. If this is not an issue anymore, we can drop the changes in this patch. Kevin 2017-04-08 1:39 GMT+08:00 Honnappa Nagarahalli < honnappa.nagaraha...@linaro.org>: > On 7 April 2017 at 07:29, 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. 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) < > > 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:kev > in.w...@arm.com>> > >>> Reviewed-by: Ola Liljedahl <ola.liljed...@arm.com<mailto: > > ola.liljed...@arm.com>> > > 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 > -- Thanks, Kevin
