On Tue, Apr 4, 2017 at 3:48 PM, Brian Brooks <brian.bro...@arm.com> wrote: > On 04/04 23:07:32, Maxim Uvarov wrote: >> On 04/04/17 22:51, Brian Brooks wrote: >> > On 04/04 22:01:23, Maxim Uvarov wrote: >> >> On 04/04/17 21:48, Brian Brooks wrote: >> >>> Remove erroneous assertion that is handled in later code >> >>> >> >> >> >> Patch logically incorrect. This patch adds odp_schedule_release_x(). >> >> Chunk of removing ASSERT has to go to "later code" patch. >> > >> > I'm not sure I understand. The short description describes the main >> > change and the long description adds more info about the other change. >> > Can you please explain? >> >> Patch has to do exactly one logical thing. Like fix bug X. Other patch >> might be improve Y. And if you send patch with fix X and improve Y than >> it's not correct. Or prove how Y depends on X. >> >> In this patch I did not understand how adding >> odp_schedule_release_atomic() into scheduler_test_queue_destroy() >> affects on assert inside fill_queues(). That is why I added that comment. > > OK, thanks for the explanation.
Agree with Maxim on this. As an aside, this sort of complex testing is why it's been proposed we add an odp_schedule_release_context() API that just does the right thing depending on the type of context you're in, similar to how odp_event_free() serves as a generic free for any event type. The problem with these individual release calls is what if we add a new queue context type in some future release? Now every app needs to do a three-way test, etc. > >> Maxim. >> >> > >> >> Maxim. >> >> >> >>> Signed-off-by: Kevin Wang <kevin.w...@arm.com> >> >>> Reviewed-by: Ola Liljedahl <ola.liljed...@arm.com> >> >>> --- >> >>> test/common_plat/validation/api/scheduler/scheduler.c | 11 +++++++++-- >> >>> 1 file changed, 9 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/test/common_plat/validation/api/scheduler/scheduler.c >> >>> b/test/common_plat/validation/api/scheduler/scheduler.c >> >>> index 952561cd..bc486192 100644 >> >>> --- a/test/common_plat/validation/api/scheduler/scheduler.c >> >>> +++ b/test/common_plat/validation/api/scheduler/scheduler.c >> >>> @@ -251,7 +251,10 @@ void scheduler_test_queue_destroy(void) >> >>> CU_ASSERT_FATAL(u32[0] == MAGIC); >> >>> >> >>> odp_buffer_free(buf); >> >>> - odp_schedule_release_ordered(); >> >>> + if (qp.sched.sync == ODP_SCHED_SYNC_ATOMIC) >> >>> + odp_schedule_release_atomic(); >> >>> + else if (qp.sched.sync == ODP_SCHED_SYNC_ORDERED) >> >>> + odp_schedule_release_ordered(); >> >>> >> >>> CU_ASSERT_FATAL(odp_queue_destroy(queue) == 0); >> >>> } >> >>> @@ -478,6 +481,11 @@ void scheduler_test_groups(void) >> >>> odp_schedule_group_leave(mygrp1, &mymask); >> >>> odp_schedule_group_leave(mygrp2, &mymask); >> >>> >> >>> + if (qp.sched.sync == ODP_SCHED_SYNC_ATOMIC) >> >>> + odp_schedule_release_atomic(); >> >>> + else if (qp.sched.sync == ODP_SCHED_SYNC_ORDERED) >> >>> + odp_schedule_release_ordered(); >> >>> + >> >>> /* Done with queues for this round */ >> >>> CU_ASSERT_FATAL(odp_queue_destroy(queue_grp1) == 0); >> >>> CU_ASSERT_FATAL(odp_queue_destroy(queue_grp2) == 0); >> >>> @@ -959,7 +967,6 @@ static void fill_queues(thread_args_t *args) >> >>> } >> >>> >> >>> ret = odp_queue_enq(queue, ev); >> >>> - CU_ASSERT_FATAL(ret == 0); >> >>> >> >>> if (ret) >> >>> odp_buffer_free(buf); >> >>> >> >> >>
