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