On 8 April 2017 at 11:24, Kevin Wang <[email protected]> wrote: > 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. The problem with this test is that it creates a queue and then expects to be able to enqueue 10000 events on the queue. The current default queue size in the scalable scheduler is 4096. 4096 minimum size Ethernet frames is 275ms, I think that's long time for a packet to sit in a queue.
The correction should be to use the new feature to request a minimum queue size when creating a queue. > > 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 > <[email protected]>: >> >> 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 > > > > > -- > Thanks, > Kevin
