> -----Original Message-----
> From: Ola Liljedahl [mailto:ola.liljed...@linaro.org]
> Sent: Sunday, April 09, 2017 12:31 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> labs.com>
> Cc: Kevin Wang <kevin.w...@linaro.org>; 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
>
> 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.savolainen@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.
Sure, if the test has a bug, it should be corrected. I was not talking about
that ... see under.
> 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.
>
> > 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.
The issue I was talking about is adding an extra release context where (by the
spec) adding it (before a queue destroy call) does not have any affect. So, why
that was added? A bug some where ?
@@ -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 ?
-Petri
>
> >
> > 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.savolainen@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
